DM-56027: Derive the rubin-env file as a dependency rsp build - #119
DM-56027: Derive the rubin-env file as a dependency rsp build#119roceb wants to merge 3 commits into
Conversation
Generated with AI Co-Authored-By: SLAC AI
timj
left a comment
timj
left a comment
There was a problem hiding this comment.
I've had a quick look and made some comments. In particular more robust URI parsing.
I also asked Codex about it and it seems to think that the code should be handling file and http URIs to be robust about what conda can do:
- [P1] Accept file:// package URLs — derive_rubinenv_env.py:76 treats only HTTP(S) lines as packages. Conda explicit files can contain file:// entries; the supported prerelease path at jenkins_wrapper.sh:130 installs locally built rubin-env packages from a file channel. Such a root is omitted from urls, causing the missing-root check to fail. Non-root file packages would instead be copied unconditionally as “header” lines at derive_rubinenv_env.py:209, potentially retaining RSP-only packages and unusable local paths. Package detection should recognize valid URL schemes, with a file:// regression test.
| the exact versions the products were built against, and excludes RSP-only | ||
| packages because they are not reachable from rubin-env. | ||
|
|
||
| Do not reintroduce the previous approach of taking package *names* from a |
There was a problem hiding this comment.
Claude needs to rephrase this to indicate to no longer refer to previous approaches but to make an explicit statement saying that the simpler approach of directly subsetting rubin-env into rubin-env-rsp does not work.
| solve, versions/build strings substituted in from the rubin-env-rsp build env. | ||
| A build variant chosen by the rsp solve can require packages the rubin-env | ||
| solve never pulled in, so the result was not dependency closed. Concretely, | ||
| rsp resolved libopencv to a qt6_* build while plain rubin-env resolved it to a |
There was a problem hiding this comment.
This is restating the same issue stated in the file itself. Likely don't need full description here.
|
|
||
|
|
||
| def run(env, *extra): | ||
| return subprocess.run( |
There was a problem hiding this comment.
If we get a third python script in this package we should consider turning it into a python package to make it easier to test without calling subprocess (in theory the tests could import the script directly and use the APIs without the subprocess).
| return seen | ||
|
|
||
|
|
||
| def main(): |
There was a problem hiding this comment.
There is too much logic in this function and it makes it hard to test. Follow the standard DM conventions where argument parsing is in its own function and the main is then mostly reading of the arguments and passing of those arguments to the function that does the real work.
| trailing '-<version>-<build>' fields. Package names may contain '-', so trim | ||
| the two trailing fields rather than splitting on '-'. | ||
| """ | ||
| fn = line.split("#", 1)[0].strip().rsplit("/", 1)[-1] |
There was a problem hiding this comment.
Might be simpler (or at least much clearer) to use urllib.parse.urlparse
| fn = line.split("#", 1)[0].strip().rsplit("/", 1)[-1] | ||
| for suffix in ARCHIVE_SUFFIXES: | ||
| if fn.endswith(suffix): | ||
| fn = fn[: -len(suffix)] |
There was a problem hiding this comment.
fn = fn.removesuffix(suffix) is better I think.
| @@ -0,0 +1,271 @@ | |||
| """Tests for derive_rubinenv_env.py. | |||
There was a problem hiding this comment.
Do the tests have a conda file that looks like a real conda file? I see lots of "http" checks in the code but none in the tests.
conda explicit files can reference a local channel, and the prereleased rubin-env path in jenkins_wrapper.sh builds rubin-env from one, so the build record carries file:// URLs. Those were invisible to the package scan: a file:// root failed the missing-root check, and non-root file:// packages were copied through as header lines, retaining RSP-only entries. Parse lines with urllib.parse.urlparse and accept http/https/file. Also split argument parsing out of main() per DM convention, and trim the comments and docstrings. Generated with AI Co-Authored-By: SLAC AI
Add tests/data with the rubin-env-rsp build record and the ${tag}.env the
old two-solve derivation produced from it, both taken from
https://eups.lsst.cloud/stack/src/env. Vendored rather than fetched so the
suite does not depend on eups.lsst.cloud being reachable from CI.
These exercise the parsers against a file conda actually wrote: 1030 package
lines over both .conda and .tar.bz2, no '#md5' fragment, and names like
_x86_64-microarch-level-4-3_icelake and rubin-env-nosysroot. The pair also
records the bug in real data -- the published file keeps the qt6_* libopencv
build with no qt6-main, and lists six packages the build env never contained.
Import the script directly for the parser tests instead of shelling out.
Generated with AI
Co-Authored-By: SLAC AI
2 participants
No description provided.