-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cram tests build-path-prefix-map substitutions #11366
base: main
Are you sure you want to change the base?
Conversation
7a06ff3
to
60a1248
Compare
$ echo $HOME | ||
$HOME | ||
/HOME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(documentation was wrong, no $
is added in front of the replacement)
Env.add env ~var:Env.Var.temp_dir ~value:(Path.to_absolute_filename path) | ||
| None, true -> Dtemp.add_to_env env | ||
| None, false -> env | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgrinberg There's a comment in the code indicating that janestreet uses set_temp_dir_when_running_actions
internally. Setting it to false
would accidentally use the correct $TMPDIR
set by cram in the env, instead of overriding the path with a wrong one. I believe the new code preserves the old behavior, but is there any chance the new ?temp_dir
would help get rid of the ref on your side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this global variable anymore internally. Feel free to get rid of it here as well I suppose. However, let's try to keep the PR's as focused as possible, so that would work should come in a separate PR.
in | ||
loop () | ||
;; | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is dead code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. Do you mind moving this change to a separate PR?
@@ -11,8 +11,6 @@ Testing install actions | |||
$ build_pkg test | |||
foobar | |||
|
|||
$ export BUILD_PATH_PREFIX_MAP="/PKG_ROOT=test/target:$BUILD_PATH_PREFIX_MAP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rgrinberg Do you know if this line is still useful somehow? The previous substitutions would only consider absolute paths, so test/target
was ignored. The new code just replaces any path anywhere, so it introduces a change on line 38 below. Should I rather ignore relative path in BUILD_PATH_PREFIX_MAP
to preserve the old behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is fine, but it needs to be behind a version check. I'd hate to break existing builds for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased and added a version check for the change of semantics
To motivate the changes a bit more, here's the before/after of the added test: $ echo /nest/$PWD/sub/dir
- /nest//home/art-w/dune/_build/.sandbox/3df9ecef4c5322405be4332860ac3ec2/default/test/blackbox-tests/test-cases/cram/sub/dir
+ /nest/$TESTCASE_ROOT/sub/dir
$ echo "[\"$PWD\",\"$PWD\"]"
- ["$TESTCASE_ROOT","/home/art-w/dune/_build/.sandbox/3df9ecef4c5322405be4332860ac3ec2/default/test/blackbox-tests/test-cases/cram"]
+ ["$TESTCASE_ROOT","$TESTCASE_ROOT"]
$ ls -a $TMPDIR
.
..
- build_3aee7a_dune
- build_6c87aa_dune
- build_785126_dune
- dune-pkg_02d959_stderr
- dune-pkg_0363ba_stderr
- dune-pkg_055d04_stderr
- ... a lot more stuff because $TMPDIR was shared
$ echo $TMPDIR
- /tmp/build_b5b53d_dune
+ $TMPDIR
$ echo $TMPDIR/sub/dir
- /tmp/build_b5b53d_dune/sub/dir
+ $TMPDIR/sub/dir
$ echo The tempdir is at:$TMPDIR:
- The tempdir is at:/tmp/build_b5b53d_dune:
+ The tempdir is at:$TMPDIR:
$ echo /this/$SPACED/issue
- /this/path/contains spaces/.but/it's not an/issue
+ /this/$SPACED/issue
$ echo $BUILD_PATH_PREFIX_MAP
- $LEFT=$RIGHT:$SPACED=path/contains spaces/.but/it's not an:HOME=HOME:/workspace_root=/home/art-w/dune/_build/.sandbox/3df9ecef4c5322405be4332860ac3ec2:$TESTCASE_ROOT=$TESTCASE_ROOT:$TMPDIR=$TMPDIR:$RIGHT=$RIGHT
+ $LEFT=$RIGHT:$SPACED=$SPACED:HOME=HOME:/workspace_root=/workspace_root:$TESTCASE_ROOT=$TESTCASE_ROOT:$TMPDIR=$TMPDIR:$RIGHT=$RIGHT |
40dc5e1
to
307645b
Compare
Signed-off-by: ArthurW <[email protected]>
Signed-off-by: ArthurW <[email protected]>
307645b
to
bf5d281
Compare
I ran into a tiny issue with cram tests output, where only the first
$TESTCASE_ROOT
path would be normalized to hide the dynamic path (using bindings from$BUILD_PATH_PREFIX_MAP
), but not the other paths on the same line. It turns out that dune expected paths to be separated by whitespace, and I was producing a json list of filenames with no spaces anywhere (withjq -c
). In retrospect this PR should also fix reproducibility issues when a parent directory contains spaces.Furthermore, the
$TMPDIR
variable was also expected to be normalized in command outputs by the build-path-prefix-map, but it wasn't matching asProcess.run
was overriding the cram test$TMPDIR
env var for a different path (the sharedDtemp.temp_dir
which contains a bunch of other things).