8000 fix: relative paths in dune-package by rgrinberg · Pull Request #5394 · ocaml/dune · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: relative paths in dune-package #5394

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
Merged
merged 5 commits into from
Feb 8, 2022
Merged

fix: relative paths in dune-package #5394

merged 5 commits into from
Feb 8, 2022

Conversation

rgrinberg
Copy link
Member

use relative paths for package sections

@bobot why do we need to include the sections in the dune-package at all?

@rgrinberg rgrinberg requested a review from bobot January 31, 2022 02:14
@bobot
Copy link
Collaborator
bobot commented Jan 31, 2022

We have no guaranty that the destination of the sections during installation have not been set, with -share for example, to something different than the default one. For lib it should be always ., but it is the only one that is fixed by the location of the dune-package.

bobot
bobot approved these changes Jan 31, 2022
View reviewed changes
Copy link
Collaborator
@bobot bobot left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get what was exactly the bug, so if it touches only files in _buildthe fix is ok. Otherwise we should really take time to change Path.reach for external path (there is an open bug for it also related to sites #5322 )

src/dune_rules/dune_package.ml
; field_l "sections" (pair Section.encode string) sections
; field_l "sections"
(pair Section.encode (Dpath.Local.encode ~dir))
(Section.Map.to_list sections)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to Path.reach keeping external path unmodified, it will not changed installed dune-package.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the tests demonstrate that it does. So I guess we aren't using external paths here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are looking at dune-package in _build/install not really installed dune-package in opam switch or /usr.

@rgrinberg
Copy link
Member Author

Consider the error in #5393

There's failures like:

Error: File unavailable:
/Users/runner/.opam/4.13.1/.opam-switch/build/mdx.2.0.0/_build/install/default/lib/mdx/mdx__Part.cmt
-> required by alias test/blackbox-tests/test-cases/mdx-stanza/runtest

Because we're using incorrect absolute paths in the dune-package file.

Otherwise we should really take time to change Path.reach for external path (there is an open bug for it also related to sites #5322 )

Are you talking about external paths to files in the _build dir? I don't think that works.
8000

@bobot
Copy link
Collaborator
bobot commented Feb 1, 2022

No, I'm talking about the installed dune-package, the path inside are external so the current fix doesn't apply and they will stay absolute, since Path.reach is the identity for external path

@bobot
Copy link
Collaborator
bobot commented Feb 1, 2022

For #5393, I think the problem is more with the substitution of the path in the dune-package during installation. Perhaps again a problem with using install file for opam, I will look into it.

@bobot
Copy link
Collaborator
bobot commented Feb 1, 2022

It is possible that #4861 was wrong because dune-site is not the only one to need the substitution.

@bobot bobot mentioned this pull request Feb 1, 2022
Closed
@rgrinberg
Copy link
Member Author

No, I'm talking about the installed dune-package, the path inside are external so the current fix doesn't apply and they will stay absolute, since Path.reach is the identity for external path

What paths are external? I don't see any external paths in the dune-package after this PR.

@rgrinberg rgrinberg added the requires-team-discussion This topic requires a team discussion label Feb 1, 2022
@rgrinberg rgrinberg added this to the 3.0.0 milestone Feb 1, 2022
@rgrinberg rgrinberg force-pushed the ps/rr/fix__relative_paths_in_dune_package branch from f29d1b7 to 8ecb536 Compare February 3, 2022 16:32
@rgrinberg
Copy link
Member Author

@bobot I added a couple of commits with external package deps, and the relative paths seem to fix the issue.

@rgrinberg
Copy link
Member Author

ping @bobot

@rgrinberg rgrinberg removed the requires-team-discussion This topic requires a team discussion label Feb 7, 2022
@bobot bobot force-pushed the ps/rr/fix__relative_paths_in_dune_package branch from cf5f38a to 915ff37 Compare February 8, 2022 10:40
@bobot
Copy link
Collaborator
bobot commented Feb 8, 2022

Thanks for the ping, I added the tests from #5404 and a test for share.

@bobot bobot force-pushed the ps/rr/fix__relative_paths_in_dune_package branch from 915ff37 to cd5592c Compare February 8, 2022 10:44
rgrinberg and others added 5 commits February 8, 2022 12:29
@rgrinberg @bobot
test: add missing mdx deps
4cfee1c 
ps-id: F3A61F3B-1DE0-4B25-BEC9-22AEB65806FF
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg @bobot
test: add cinaps as a dependency
da09945 
ps-id: CB2B45B9-655B-4E2B-858A-70BD7F013C83
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
@rgrinberg @bobot
fix: relative paths in dune-package
8000
f05ae51 
use relative paths for package sections

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ps-id: 61A22807-300A-408D-B3A0-D9E4D6A2A39E
@bobot
[Test] Check dune-package after installation
8421fad 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot
[Tests] Add a file to install in share to find in dune-package
9b6c677 
Signed-off-by: François Bobot <francois.bobot@cea.fr>
@bobot bobot force-pushed the ps/rr/fix__relative_paths_in_dune_package branch from cd5592c to 9b6c677 Compare February 8, 2022 11:29
@rgrinberg
Copy link
Member Author

Thanks. The tests look good.

@rgrinberg rgrinberg merged commit 1a62546 into ocaml:main Feb 8, 2022
@rgrinberg rgrinberg deleted the ps/rr/fix__relative_paths_in_dune_package branch February 8, 2022 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobot bobot bobot approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@rgrinberg @bobot
0