Skip to content

dune-site should replace paths in "sections", not "sites"#5192

Merged
bobot merged 3 commits intoocaml:mainfrom
strub:fix-4389
Nov 20, 2021
Merged

dune-site should replace paths in "sections", not "sites"#5192
bobot merged 3 commits intoocaml:mainfrom
strub:fix-4389

Conversation

@strub
Copy link
Copy Markdown
Contributor

@strub strub commented Nov 17, 2021

This should fix #4389

@ejgallego
Copy link
Copy Markdown
Collaborator

Merci @strub , @bobot , is this a candidate for 2.9.2 ?

@bobot
Copy link
Copy Markdown
Collaborator

bobot commented Nov 17, 2021

Great thank you! Do you think you can also add a test?

@ejgallego ejgallego requested a review from bobot November 17, 2021 17:16
@strub
Copy link
Copy Markdown
Contributor Author

strub commented Nov 17, 2021

Having it in dune 2.9.2 would be great. I can try to write a test.

@strub strub marked this pull request as draft November 17, 2021 21:40
@strub
Copy link
Copy Markdown
Contributor Author

strub commented Nov 18, 2021

In fact, the system is badly broken because of a bad interaction between the sites substitution and the version substitution. when the version is not needed or unavailable, the site substitution is totally ignored. I discovered this while writing the test. I am going to fix this first.

@strub strub marked this pull request as ready for review November 18, 2021 09:08
@strub
Copy link
Copy Markdown
Contributor Author

strub commented Nov 18, 2021

Ok, I think I'm done with the fix & the test case.

Copy link
Copy Markdown
Collaborator

@bobot bobot left a comment

Choose a reason for hiding this comment

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

Thank you a lot. The branch is nearly good to go:

  • Could you add a mention of the fix in Changes ?
  • And (but I could do it if you don't have time) could you move the addition of the test before the fix, in order to see the improvement?

@ejgallego
Copy link
Copy Markdown
Collaborator

ejgallego commented Nov 18, 2021

@bobot if you want this in 2.9.2 then please set the milestone and place the changes entry in the right place so we don't forget.

@bobot bobot added this to the 2.9.2 milestone Nov 18, 2021
@bobot
Copy link
Copy Markdown
Collaborator

bobot commented Nov 19, 2021

@jeremiedimino Why in e16fca0 failure in reparsing the dune-package during installation was considered as a warning instead of as a Code_error? Only dune produce such files, no ?

@strub I just modified your commits to update the tests so that the error is clear in the first commit. I will merge soon if you don't have any remark. Thanks a lot for the fix!

@bobot
Copy link
Copy Markdown
Collaborator

bobot commented Nov 19, 2021

@ejgallego the change is moved to 2.9.2

@strub
Copy link
Copy Markdown
Contributor Author

strub commented Nov 19, 2021

@bobot No remarks. You can merge!

@ejgallego
Copy link
Copy Markdown
Collaborator

@ejgallego the change is moved to 2.9.2

Great, thanks; will backport myself; if there is any problem I'll ping you guys. There is one other pending fix for 2.9.2 too, so I hope to release next week.

@bobot bobot self-assigned this Nov 20, 2021
@bobot bobot enabled auto-merge (rebase) November 20, 2021 21:14
strub and others added 3 commits November 21, 2021 00:34
Signed-off-by: Pierre-Yves Strub <pierre-yves@strub.nu>
Signed-off-by: François Bobot <francois.bobot@cea.fr>
 - dune-site should replace paths in "sections", not "sites"
 - paths' substitution should be done in all cases (not only
   when a version is added to dune-package)

fix #4389

Signed-off-by: Pierre-Yves Strub <pierre-yves@strub.nu>
@bobot bobot merged commit 0281cf2 into ocaml:main Nov 20, 2021
@ejgallego
Copy link
Copy Markdown
Collaborator

@bobot I need help to create a 2.9 version of the changes in install_uninstall.ml:copy_special_file , unfortunately I'm at a loss here on how to backport.

@bobot
Copy link
Copy Markdown
Collaborator

bobot commented Jan 20, 2022

Ok , how do you want to proceed?

@ejgallego
Copy link
Copy Markdown
Collaborator

We moved the discussion to slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to parse file, not adding version and locations information

3 participants