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

.version: don't read from .version and deduplicate .version-suffix references #39416

Merged
merged 4 commits into from Apr 30, 2018

Conversation

@Ma27
Copy link
Member

@Ma27 Ma27 commented Apr 24, 2018

Motivation for this change

See commit messages for further details.
The following changes were done:

  • Reverted newline fix I introduced this morning (using fileContents the problem doesn't exist anymore)
  • Deduplicated suffix logic (reading .version and .version-suffix is implemented in nixos/version and lib/trivial.nix).
  • Fixed .version reference in the pkgs.osquery derivation (using stdenv.lib.nixpkgsVersion now)

Running nix-build nixos/release.nix produces an output path with the proper version and suffix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 24, 2018

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: osquery

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.


Ma27 referenced this pull request Apr 24, 2018
When reading the `nixpkgs` version from `.version` you always have a
`\n` at the end because of the final newline.

This issue exists since b7d15ed and had to be fixed several times
according to the history of `.version`.

Furthermore @Infinisil recommended I explicitly configured
`.editorconfig` to avoid newlines in `.version`.
@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 24, 2018

Success on x86_64-linux (full log)

Attempted: osquery

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2
shrinking /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2/bin/osqueryd
shrinking /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2/bin/osqueryi
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2/bin
patching script interpreter paths in /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2
/nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2/etc/init.d/osqueryd: interpreter directive changed from "/bin/sh" to "/nix/store/xn5gv3lpfy91yvfy9b0i7klfcxh9xskz-bash-4.4-p19/bin/sh"
checking for references to /build in /nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2...
/nix/store/zih5v86ddlspnz6srbpmh5nhffcyvfb0-osquery-3.2.2

lib/trivial.nix Outdated
let suffixFile = ../.version-suffix; in
fileContents ../.version
+ (if pathExists suffixFile then fileContents suffixFile else "pre-git");
nixpkgsVersion = version + suffix;

This comment has been minimized.

@fpletz

fpletz Apr 24, 2018
Member

I think we should adopt a similar naming as the system.nixos NixOS module is using.

Suggestion:

  • version -> release
  • suffix -> versionSuffix
  • nixpkgsVersion -> version

Maybe prefix some or all with nixpkgs or nixos or split out an extra lib namespace for this kind of information?

Any more ideas?

This comment has been minimized.

@dezgeg

dezgeg Apr 28, 2018
Contributor

A problem with the renaming is that e.g. the nix repo itself is referring to nixpkgsVersion, so it will become really painful.

This comment has been minimized.

@dezgeg

dezgeg Apr 28, 2018
Contributor

I guess you could keep a compatibility alias for nixpkgsVersion at least.

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Apr 26, 2018

@fpletz I just did the renames you suggested (and @globin agreed on). I thought that the ACK of two core maintainers should be sufficiently fair, it's currently in another commit to make reverts easier in case several folks are against that change %)

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 26, 2018

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: osquery

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.


@globin
globin approved these changes Apr 26, 2018
@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 26, 2018

Success on x86_64-linux (full log)

Attempted: osquery

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2
shrinking /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2/bin/osqueryd
shrinking /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2/bin/osqueryi
strip is /nix/store/j75dgadrff2d1fyc4fczmcgqkid2imdx-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2/bin
patching script interpreter paths in /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2
/nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2/etc/init.d/osqueryd: interpreter directive changed from "/bin/sh" to "/nix/store/xn5gv3lpfy91yvfy9b0i7klfcxh9xskz-bash-4.4-p19/bin/sh"
checking for references to /build in /nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2...
/nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2

@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Apr 28, 2018

@dezgeg I guess you're right, we shouldn't break this espcially when it's in lib and nix references to this attribute.
I'll adjust my last commit to retain lib.nixpkgsVersion as alias with deprecation warning and write about it in the release notes :-)

@Ma27 Ma27 force-pushed the Ma27:fix-.version-config branch Apr 28, 2018
Ma27 added 4 commits Apr 24, 2018
This way easier to understand and the officially recommended approach.

/cc @dezgeg @fpletz
The logic regarding the generated `.version-suffix` file is already
defined in `lib/trivial.nix` and shouldn't be duplicated in
`nixos/version`.
As suggested in #39416 (comment)
the versioning attributes in `lib` should be consistent to
`nixos/version` which implicates the following changes:

* `lib.trivial.version` -> `lib.trivial.release`
* `lib.trivial.suffix` -> `lib.trivial.versionSuffix`
* `lib.nixpkgsVersion` -> `lib.version`

As `lib.nixpkgsVersion` is referenced several times in `NixOS/nixpkgs`,
`NixOS/nix` and probably several user's setups. As the rename will cause
a notable impact it's better to keep `lib.nixpkgsVersion` as alias with
a warning yielded by `builtins.trace`.
@Ma27 Ma27 force-pushed the Ma27:fix-.version-config branch to 9274ea3 Apr 28, 2018
@Ma27
Copy link
Member Author

@Ma27 Ma27 commented Apr 28, 2018

I added an alias as @dezgeg suggested, furthermore I rebased onto master as my changes in the release notes caused a conflict with master.

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 28, 2018

Success on x86_64-linux (full log)

Attempted: osquery

Partial log (click to expand)

/nix/store/c5lsaw4ixz0h315fys14bzrz4d6z53ya-osquery-3.2.2

@GrahamcOfBorg
Copy link

@GrahamcOfBorg GrahamcOfBorg commented Apr 28, 2018

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: osquery

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.


@7c6f434c 7c6f434c merged commit fd8dcdf into NixOS:master Apr 30, 2018
8 checks passed
8 checks passed
@GrahamcOfBorg
grahamcofborg-eval ^.^!
Details
@GrahamcOfBorg
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@GrahamcOfBorg
grahamcofborg-eval-nixos-manual nix-instantiate ./nixos/release.nix -A manual
Details
@GrahamcOfBorg
grahamcofborg-eval-nixos-options nix-instantiate ./nixos/release.nix -A options
Details
@GrahamcOfBorg
grahamcofborg-eval-nixpkgs-manual nix-instantiate ./pkgs/top-level/release.nix -A manual
Details
@GrahamcOfBorg
grahamcofborg-eval-nixpkgs-tarball nix-instantiate ./pkgs/top-level/release.nix -A tarball
Details
@GrahamcOfBorg
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate ./pkgs/top-level/release.nix -A unstable
Details
@GrahamcOfBorg
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@Ma27 Ma27 deleted the Ma27:fix-.version-config branch Apr 30, 2018
Synthetica9 added a commit to Synthetica9/nixpkgs that referenced this pull request May 3, 2018
As suggested in NixOS#39416 (comment)
the versioning attributes in `lib` should be consistent to
`nixos/version` which implicates the following changes:

* `lib.trivial.version` -> `lib.trivial.release`
* `lib.trivial.suffix` -> `lib.trivial.versionSuffix`
* `lib.nixpkgsVersion` -> `lib.version`

As `lib.nixpkgsVersion` is referenced several times in `NixOS/nixpkgs`,
`NixOS/nix` and probably several user's setups. As the rename will cause
a notable impact it's better to keep `lib.nixpkgsVersion` as alias with
a warning yielded by `builtins.trace`.
@edolstra

This comment has been minimized.

Copy link
Member

@edolstra edolstra commented on 9274ea3 Aug 3, 2018

To be honest, I'm not really fond of such renames since they just cause pain for users (i.e. they break evaluation and/or produce annoying deprecation warnings).

See e.g. NixOS/nix#2291: we either have to tell people to use nixpkgsVersion (which won't work with older releases) or use version (which causes a deprecation warning and will presumably stop working in the future).

This comment has been minimized.

Copy link
Member Author

@Ma27 Ma27 replied Aug 3, 2018

@edolstra thanks for your feedback! Just to get this straight, are you simply against the deprecation warning for lib.nixpkgsVersion or do you have an issue with the renaming overall?

This comment has been minimized.

Copy link
Member

@edolstra edolstra replied Aug 3, 2018

Mostly the deprecation warning, but the name lib.version is also not obviously better than lib.nixpkgsVersion so it's not clear whether it's worth the effort to rename.

This comment has been minimized.

Copy link
Member Author

@Ma27 Ma27 replied Aug 3, 2018

Thanks for elaborating. I'm currently busy with other stuff, but I'd file another patch on the weekend to revert the warning and ping you.
Regarding the actual rename: @fpletz suggested to make the naming in lib consistent wiht the naming in the NixOS modules (see discussion at #39416 (comment)).

For me a consistent naming for all version-related stuff in nixpkgs seems quite good, but I guess that it might be better to discuss it in the PR with all participants as this was this place where the suggestion was made :)

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Aug 5, 2018
Originally introduced in NixOS#39416 (1),
however it causes confusion and shouldn't be deprecated (2)(3).

/cc @edolstra

(1) NixOS#39416 (comment)
(2) NixOS@9274ea3#commitcomment-29951594
(3) NixOS/nix#2291
@FRidh
Copy link
Member

@FRidh FRidh commented Jul 13, 2019

It happened now several times that lib.version ended up in a derivation because rec was not used. Yes, the use of with lib can be dangerous, but obtaining a version attribute from it is really annoying. We should not bring everything that is in lib in a certain namespace into top-level lib.

I very much want to get rid of lib.version in #64693.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants