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

Add info for nixos (opam2nix) #14477

Merged
merged 7 commits into from
Feb 7, 2020
Merged

Add info for nixos (opam2nix) #14477

merged 7 commits into from
Feb 7, 2020

Conversation

bobot
Copy link
Contributor

@bobot bobot commented Jul 11, 2019

Mainly add nixos precise packages, for use with @timbertson's opam2nix.

packages/zarith/zarith.1.7/opam Outdated Show resolved Hide resolved
@camelus
Copy link
Contributor

camelus commented Jul 11, 2019

Commit: 6efed3f

@bobot has posted 19 contributions.

🌩️ opam-lint errors 6efed3f
  • conf-pkg-config.1.1 has errors:

    • error 46: Package is flagged "conf" but has source, install or remove instructions
  • camlzip.1.07 has some warnings:

    • warning 37: Missing field 'dev-repo'
  • coq.8.3 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coq.8.4.5 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.4.6 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.4pl1 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coq.8.4pl2 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coq.8.4pl4 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coq.8.5.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.5.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.5.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.5.3 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.6.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.6 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.7.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.7.1+1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.7.1+2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.7.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.7.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.8.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.8.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coq.8.8.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.4.5 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coqide.8.4.6 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coqide.8.4pl2 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coqide.8.4pl4 has some warnings:

    • warning 25: Missing field 'authors'
    • warning 35: Missing field 'homepage'
    • warning 36: Missing field 'bug-reports'
    • warning 37: Missing field 'dev-repo'
  • coqide.8.5.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.5.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.5.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.5.3 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.6.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.6 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.7.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.7.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.7.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.8.0 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.8.1 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • coqide.8.8.2 has some warnings:

    • warning 47: Synopsis (or description first line) should start with a capital and not end with a dot
  • These packages passed lint tests: conf-expat.1, conf-gcc.1.0, conf-glade.2, conf-gnomecanvas.2, conf-gtksourceview.2, coq.8.9.0, coq.8.9.1, coqide.8.9.0, coqide.8.9.1, lablgtk.2.18.6, zarith.1.7-1, zarith.1.9.1


☀️ Installability check (+2)
  • new installable packages (2): conf-gcc.1.0 zarith.1.7-1

🌤️ 1 ignored non-opam files:
  • packages/zarith/zarith.1.7-1/files/absolute_CC.patch

@timbertson
Copy link
Contributor

Thanks @bobot ! The os-distribution opam2nix declares is nixpkgs though, not nixos: https://github.com/timbertson/opam2nix/blob/c4c66eda70e40998180eb086aa6dbcc4d92dd83b/src/opam_metadata.ml#L107

(I debated this, but nixpkgs is the name of the package set, so it seemed the most appropriate)

@bobot
Copy link
Contributor Author

bobot commented Jul 12, 2019

There is a problem in my proposition, I followed other packages which uses "nixos" as the name of the os for depext. But opam2nix still uses "nixpkgs". @timbertson What is the right name to use?

I'm going to add for now "nixpkgs" as a copy of "nixos" dependencies it could be removed once the name is choosen.

EDIT: Sorry the comments updated after I send this. My comments is outdated.

@bobot
Copy link
Contributor Author

bobot commented Jul 12, 2019

The os-distribution opam2nix declares is nixpkgs though, not nixos:
(I debated this, but nixpkgs is the name of the package set, so it seemed the most appropriate)

It should be clarified in opam which uses in some way both names: https://github.com/ocaml/opam/blob/master/src/state/opamFormatUpgrade.ml#L24

For now I'm going to duplicate all the lines.

@@ -23,6 +23,7 @@ depexts: [
["opus-devel"] {os-distribution = "fedora"}
["opus-devel"] {os-family = "suse"}
["libopus"] {os-distribution = "nixos"}
Copy link
Member

@kit-ty-kate kit-ty-kate Jul 12, 2019

Choose a reason for hiding this comment

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

Could you remove the nixos line if it's not used instead? (it's definitely not used by opam-depext)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is true that depext doesn't detect this os. But there is code in opam (for upgrade) which take into account nixos (and prefers it compared to nix). Does people use nixos themselves in depext?

I can remove the nixos line, I have no opinion on the previous question.

Do we have a list of the nomenclature for os-family, os, os-distribution?

Copy link
Member

Choose a reason for hiding this comment

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

I clarify this in #14477 (comment)
You can see how opam detects distribution/os/os-family here: https://github.com/ocaml/opam/blob/c35eb977d19457d3f13b31c1aa60de4fb210865d/src/state/opamSysPoll.ml

What is the ID field in /etc/os-release in NixOS?

@kit-ty-kate
Copy link
Member

It should be clarified in opam which uses in some way both names: https://github.com/ocaml/opam/blob/master/src/state/opamFormatUpgrade.ml#L24

This is only the format upgrade module, some parts are not actually used by opam-depext. However, if it were to be used at some point this needs to match the ID field in /etc/os-release (if your distribution has it)

@timbertson
Copy link
Contributor

It should be clarified in opam which uses in some way both names: https://github.com/ocaml/opam/blob/master/src/state/opamFormatUpgrade.ml#L24

Uh, that code is just wrong I'm afraid. opam2nix has always used nixpkgs, never nixos. I don't know why this code exists, someone must have gotten confused.

@bobot
Copy link
Contributor Author

bobot commented Jul 12, 2019

Soemone perhaps used opam in nixos directly (like opam on ubuntu, ...). It is a different use from opam2nix.

@bobot
Copy link
Contributor Author

bobot commented Jul 12, 2019

@timbertson I have some problems with os-family. I'm on debian so os-family = "debian", which is used in some depexts, is true for me. OpamSysPoll.variables () defines arch, os, os-distribution, os-version, os-family. Only the first two should be used (white list, instead of blacklist). I'm going to make an MR on opam2nix.

I will recheck this MR.

@timbertson
Copy link
Contributor

I'd like to find out where (if anywhere) nixos is actually used. Duplicating deps for both nixos and nixpkgs is just going to get confusing.

@AltGr , It looks like the first introduction of nixos was in ocaml/opam@e236be5. I don't suppose you remember?

From the comments in the related issue "NixOs" was casually mentioned, I'm wondering if that accidental inaccuracy simply made its way into the code unchecked? Or do you know of a tool which actually uses "nixos"? (opam2nix doesn't, it only uses "nixpkgs")

@bobot
Copy link
Contributor Author

bobot commented Jul 12, 2019

I'd like to find out where (if anywhere) nixos is actually used. Duplicating deps for both nixos and nixpkgs is just going to get confusing.

Indeed we should not have both nixos and nixpkgs. And I think @kit-ty-kate is right, we should look at what nixos choose for /etc/os-release, and it is nixos. Opam will by default detect this os-distribution when run on nixos. So I think opam2nix should follow that.

@timbertson
Copy link
Contributor

I use fedora with nixpkgs, so opam's builtin depext is always going to do the wrong thing there. Is it ever going to support nixos? I guess doing nix-env -ia <attr> would be the closest analogy.

Maybe using "os" for depexts is just wrong in general, the information you really want is "which package sets are available". For most distros one implies the other, but not when third-party package managers come into it. I assume OSX/Windows would have the same kind of thing, since there are a number of package managers which might be present.

@bobot
Copy link
Contributor Author

bobot commented Jul 13, 2019

I use fedora with nixpkgs, so opam's builtin depext is always going to do the wrong thing there. Is it ever going to support nixos?

Why will it always do the wrong thing? If you use opam directly depexts should use fedora package; if you use opam2nix it will use nixos package. I don't think opam directly but using nix package is a real use case thanks to opam2nix.

@timbertson
Copy link
Contributor

Sorry for stalling for so long on this. I have in the past added nixpkgs depexts in this repo, so I was under the impression there was now a mix of both nixpkgs and nixos, and I can't make opam2nix support both. But I've just re-checked, and it looks like the upgrade code has migrated all those to nixos, which makes them not work.

So since the current code doesn't have any effect since nobody's actually using nixpkgs, I'm going to just change opam2nix to look at nixos so that it works 🤷‍♂️. @bobot , want to remove all the nixpkgs lines?

While we're at it, any thoughts on os-family? It's not currently set, maybe that's best for clarity because there aren't any other members of the family.

@bobot bobot force-pushed the for_nix_opam2 branch 2 times, most recently from 9a1eb2b to ae699e3 Compare November 25, 2019 13:31
@bobot
Copy link
Contributor Author

bobot commented Nov 25, 2019

Ok done, the new version 1.7a should not be needed anymore since zarith 1.9.1 is released. But I haven't tested it in nix.

@bobot bobot requested a review from avsm November 25, 2019 13:36
timbertson added a commit to timbertson/opam2nix that referenced this pull request Nov 26, 2019
@bobot
Copy link
Contributor Author

bobot commented Dec 4, 2019

The CI seems to have been canceled. What is the error?

bobot added 3 commits January 15, 2020 16:13
  * backport handling of absolute CC
  * conf-pkg-config for nixos
  * add conf-gcc
(thanks kit-ty-kate)
@bobot
Copy link
Contributor Author

bobot commented Jan 17, 2020

The CI doesn't pass on this MR, but the opam files of the failing packages are not semantically modified. They would fail before this MR.

@kit-ty-kate
Copy link
Member

The CI was cancelled several times on this PR to avoid freezing the whole CI as it modifies a lot of packages.

Is this PR ready to be reviewed? Is there a consensus?

@bobot
Copy link
Contributor Author

bobot commented Jan 20, 2020

It has already been reviewed, it could be again. I think the most contentious point was the name of the os-distribution (nixos or nixpkgs). It has been accepted as nixos at the end. Consensus reached and implemented.

@timbertson
Copy link
Contributor

Agreed, as the source of contention for the distribution name (sorry 'bout that) I'm happy with this PR now 👍

["gcc"] {os-distribution = "debian"}
["gcc"] {os-distribution = "ubuntu"}
["gcc"] {os-distribution = "nixos"}
["gcc"] {os-distribution = "archlinux"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
["gcc"] {os-distribution = "archlinux"}
["gcc"] {os-distribution = "arch"}

Copy link
Member

@avsm avsm left a comment

Choose a reason for hiding this comment

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

Just a minor comment, but can fix that as part of #15789

@avsm avsm merged commit a59e00c into ocaml:master Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants