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.

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

opam2 built-in os detection sets os to macos instead of darwin #3148

Closed
avsm opened this issue Jan 1, 2018 · 6 comments
Closed

opam2 built-in os detection sets os to macos instead of darwin #3148

avsm opened this issue Jan 1, 2018 · 6 comments

Comments

@avsm
Copy link
Member

avsm commented Jan 1, 2018

opam1 set the os variable in constraints to darwin (this is used in various opam packages for osx-specific config)

opam2 sets it to macos, which in turn causes build failures due to the package repository testing for os = "darwin".

We should reconcile this to be consistent between opam1 and 2

@marco-m
Copy link

marco-m commented Jan 4, 2018

+1 for darwin, which is arguably the name to use (output of uname) and, if I understand correctly, would reduce the overall number of changes.

@dbuenzli
Copy link
Contributor

dbuenzli commented Jan 4, 2018

This was discussed in #3058 I think macos is the right name especially since it is used with sw_vers -productVersion for os-version, which wouldn't make any sense for darwin.

You could argue to have darwin for os and uname -r for os-version but that's really not what you want from an end-user perspective: you want to constrain things by macOS versions.

@avsm
Copy link
Member Author

avsm commented Jan 6, 2018

I'm ok with using macos in opam2, as long as the repo rewriter logic also maps the variable name back to darwin for opam1 compatibility

@AltGr
Copy link
Member

AltGr commented Jan 9, 2018

The rewriting should have accounted for that change, it seems like a bug.

@AltGr
Copy link
Member

AltGr commented Jan 9, 2018

Ok, the rewriting was done properly for depexts, but not in other places (available, and filters). Fixing that.

@AltGr
Copy link
Member

AltGr commented Jan 9, 2018

Now fixed in the 2.0.0 branch of the repo. Note that we don't handle the cases where the os variable would have been used as a string (e.g. make "%{os}%.target". But thankfully nobody is doing that !)

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

No branches or pull requests

4 participants