Skip to content

Fixes: Env var & delimiters, unversionned opam file pin, doc#3404

Merged
rjbou merged 5 commits intoocaml:masterfrom
rjbou:fx
Jun 15, 2018
Merged

Fixes: Env var & delimiters, unversionned opam file pin, doc#3404
rjbou merged 5 commits intoocaml:masterfrom
rjbou:fx

Conversation

@rjbou
Copy link
Copy Markdown
Collaborator

@rjbou rjbou commented Jun 11, 2018 •
edited
Loading

This PR closes #3391.

  • It fixes OpamStd.split_delim implementation which had the same behaviour than OpamStd.split. Now they respectively keep & clean empty strings from the returned list.
  • An option is added to OpamStd.Sys.split_path_variable, permiiting to retrieve a cleaned path or no.
  • A fix on environment variables construction, in order to keep the trailing delimiter when it should be.

edit:

@rjbou rjbou changed the title Environment variables with trailing or leading delimiters fix Environment variables with trailing or leading delimiters (fix) Jun 12, 2018
@rjbou rjbou changed the title Environment variables with trailing or leading delimiters (fix) Fixes: Env var & delimiters, unversionned opam file pin Jun 12, 2018
@rjbou rjbou changed the title Fixes: Env var & delimiters, unversionned opam file pin Fixes: Env var & delimiters, unversionned opam file pin, json Jun 12, 2018
dbuenzli
dbuenzli reviewed Jun 12, 2018
View reviewed changes
src/core/opamJson.ml Outdated
| '\\' -> flush b start i; adds b "\\\\"; loop next next
| '\x00' .. '\x1F' | '\x7F' (* US-ASCII control chars *) as c ->
| '\x00' .. '\x1F' | '\x7F' (* US-ASCII control chars *)
| '\x80' .. '\xFF' (* Extended ASCII chars *) as c ->
Copy link
Copy Markdown
Contributor

@dbuenzli dbuenzli Jun 12, 2018

Choose a reason for hiding this comment

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

You can't do this here, this will break UTF-8 output. The module assumes strings are UTF-8 encoded.

Btw. could you please make one PR per issue you fix.

AltGr
AltGr approved these changes Jun 12, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@AltGr AltGr left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I'll trust you have checked the many corner-cases :)

src/client/opamPinCommand.ml
OpamConsole.warning
"Couldn't retrieve opam file from versionned source, \
using the one found locally.";
Some local
Copy link
Copy Markdown
Member

@AltGr AltGr Jun 12, 2018

Choose a reason for hiding this comment

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

I am wondering if the case when both are defined and different (Some local, Some vers when local <> vers; or maybe when not (OpamFile.OPAM.effectively_equal local vers)) would not deserve a warning too: "not using uncommitted version of opam file xxx".

But there is already the warning about uncommitted changes, so that might be redundant.

Copy link
Copy Markdown
Contributor

@dbuenzli dbuenzli Jun 12, 2018

Choose a reason for hiding this comment

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

I really think this "fix" muddies again the semantis of git pins and should not be merged. I don't see a clear ux benefit in doing so.

Copy link
Copy Markdown
Collaborator Author

@rjbou rjbou Jun 13, 2018

Choose a reason for hiding this comment

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

@AltGr It would be complementary: on update, uncommitted changes warning is shown, in pinning, uncommitted opam changes warning is shown.

@dbuenzli
Copy link
Copy Markdown
Contributor

dbuenzli commented Jun 12, 2018

Also on #3403 I wonder if there's really anything to fix. For me it works as expected, a git pin considers only what is in git.

@AltGr
Copy link
Copy Markdown
Member

AltGr commented Jun 12, 2018

@dbuenzli there was still a consistency problem, because then we shouldn't use the non-versionned opam files for package detection (which would actually be more involved). It was a bit weird to detect the package definition file for pinning, but then say that no definition was found.

@dbuenzli
Copy link
Copy Markdown
Contributor

dbuenzli commented Jun 12, 2018 •
edited
Loading

@dbuenzli there was still a consistency problem, because then we shouldn't use the non-versionned opam files for package detection (which would actually be more involved).

Then why do you use the non-versionned opam file at all ? Just say no opam file was found, only consider what is in the VCS.

With this the system becomes idiosyncratic to understand. Now if I have an opam VCS file with uncommited changes why won't opam take it into account while it did take into account things that were not in the VCS before ? Keep these semantics simple.

@dbuenzli
Copy link
Copy Markdown
Contributor

dbuenzli commented Jun 14, 2018 •
edited
Loading

Then why do you use the non-versionned opam file at all ? Just say no opam file was found, only consider what is in the VCS.

See also #3408

I really think the way this is done too complicated and should be carefully reviewed (better, documented so that one can see if one can explains the behaviour in simple words, which will inform the design).

I think the confusion by @bobbypriambodo in #3403 is a confusion due to a change in defaults (and it's right to be confused by a change in defaults), I don't think it's a confusion in expectations.

@bobbypriam
Copy link
Copy Markdown

bobbypriam commented Jun 14, 2018

I think the confusion by @bobbypriambodo in #3403 is a confusion due to a change in defaults

I confirm this, it did throw me off because I remembered opam v1.2 worked that way. However I do agree with @dbuenzli that the behavior is consistent and reasonable. Perhaps it only warrants a documentation and/or a warning if opam cannot find an opam file? Something like no package definition is found -- perhaps you forgot to commit the opam file? (definitely could be better).

@rjbou
Copy link
Copy Markdown
Collaborator Author

rjbou commented Jun 14, 2018

I really think the way this is done too complicated and should be carefully reviewed (better, documented so that one can see if one can explains the behaviour in simple words, which will inform the design).

Indeed, it should be added in the new documentation. For the UX part, user is alerted by messages when uncommitted changes are discarded on upgrade, when an unversioned opam file is taken on pinning, when uncommitted opam file changes are discarded, etc.

@rjbou rjbou changed the title Fixes: Env var & delimiters, unversionned opam file pin, json Fixes: Env var & delimiters, unversionned opam file pin, doc Jun 15, 2018
@rjbou rjbou merged commit ae5bbc4 into ocaml:master Jun 15, 2018
This was referenced Apr 30, 2024
Closed
Closed
Merged
@rjbou rjbou mentioned this pull request Mar 13, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AltGr AltGr AltGr approved these changes

+1 more reviewer

@dbuenzli dbuenzli dbuenzli left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Minimal opam file incompatibility on v1.2 and v2 when pinning opam breaks man

4 participants

@rjbou @dbuenzli @AltGr @bobbypriam