Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(stdune): make sure Path.drop_prefix drops path prefixes only #8965

Merged
merged 1 commit into from
Dec 7, 2023
Merged

fix(stdune): make sure Path.drop_prefix drops path prefixes only #8965

merged 1 commit into from
Dec 7, 2023

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Oct 18, 2023

Fixes #8870

@Alizter Alizter force-pushed the ps/branch/fix_stdune___make_sure_path_drop_prefix_drops_path_prefixes_only branch from 4600cf5 to 002b14a Compare October 18, 2023 09:10
@rgrinberg rgrinberg requested a review from anmonteiro October 18, 2023 17:10
@Alizter Alizter mentioned this pull request Oct 19, 2023
Merged
@Alizter Alizter force-pushed the ps/branch/fix_stdune___make_sure_path_drop_prefix_drops_path_prefixes_only branch from 4bad250 to 711a278 Compare October 19, 2023 22:22
@rgrinberg rgrinberg marked this pull request as draft October 19, 2023 23:55
@emillon
Copy link
Collaborator

emillon commented Oct 25, 2023

FYI the issue is affecting our own test suite since in the describe tests, when replacing lib/ocaml to FINDLIB, we're replacing lib/ocaml-compiler-libs by FINDLIB/-compiler-libs. This happens with OCaml 5.1 which might have a slightly different dir layout.

@Alizter
Copy link
Collaborator Author

Alizter commented Oct 25, 2023 •
edited
Loading

@rgrinberg Why was this marked as draft? I don't think the CI errors are related, but I can do a fresh push.

@rgrinberg
Copy link
Member

b/c the tests are failing

@Alizter
Copy link
Collaborator Author

Alizter commented Oct 25, 2023

Sorry @rgrinberg you are correct that the failures are relevant. I had a look and I couldn't work out how to fix melange here. I might have a look again at a later date.

@Alizter Alizter force-pushed the ps/branch/fix_stdune___make_sure_path_drop_prefix_drops_path_prefixes_only branch from 8c3838c to c0e984b Compare November 16, 2023 19:33
@Alizter Alizter marked this pull request as ready for review November 16, 2023 19:48
@Alizter
Copy link
Collaborator Author

Alizter commented Nov 16, 2023

@rgrinberg @anmonteiro Melange is now fixed. I was missing the case where we drop the entire path which should give just Local.root. I've updated the tests to check this case too.

@Alizter
Copy link
Collaborator Author

Alizter commented Nov 21, 2023

ping

@Alizter Alizter force-pushed the ps/branch/fix_stdune___make_sure_path_drop_prefix_drops_path_prefixes_only branch from 0ba1ab0 to 415b026 Compare December 1, 2023 04:12
Alizter added a commit to Alizter/dune that referenced this pull request Dec 3, 2023
@Alizter
test: dropping entire prefix for Path.drop_prefix
8cc7904 
We add a test demonstrating that dropping the entire path as the refix
in both Path.drop_prefix and Path.External.drop_prefix has a set
behaviour. This is in preperation of ocaml#8965.

Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter mentioned this pull request Dec 3, 2023
Merged
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 3, 2023

@rgrinberg I've created #9363 for the extra tests I added here.

Alizter added a commit to Alizter/dune that referenced this pull request Dec 3, 2023
@Alizter
test: dropping entire prefix for Path.drop_prefix
6c7e50c 
We add a test demonstrating that dropping the entire path as the refix
in both Path.drop_prefix and Path.External.drop_prefix has a set
behaviour. This is in preperation of ocaml#8965.

Signed-off-by: Ali Caglayan <[email protected]>
rgrinberg pushed a commit that referenced this pull request Dec 3, 2023
@Alizter
test: dropping entire prefix for Path.drop_prefix (#9363)
8502f33 
We add a test demonstrating that dropping the entire path as the refix
in both Path.drop_prefix and Path.External.drop_prefix has a set
behaviour. This is in preperation of #8965.

Signed-off-by: Ali Caglayan <[email protected]>
@Alizter Alizter requested a review from rgrinberg December 5, 2023 21:22
@Alizter Alizter force-pushed the ps/branch/fix_stdune___make_sure_path_drop_prefix_drops_path_prefixes_only branch from 8302859 to decd1e2 Compare December 7, 2023 03:14
@Alizter
Copy link
Collaborator Author

Alizter commented Dec 7, 2023

@rgrinberg Sorry about those extra tests, not sure what happened with git but it resolved the conflict in a funny way causing the tests to be duplicated. Should all be fixed now.

@rgrinberg rgrinberg merged commit 074ca86 into ocaml:main Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro Awaiting requested review from anmonteiro

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Alizter @emillon @rgrinberg