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

stdenv: Deprecate string configureFlags #45886

Conversation

Ericson2314
Copy link
Member

Motivation for this change

This is better style, especially once we use the Nix 2.0 feature to go directly to bash arrays. #44423 already made nixpkgs largely conforming.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/i6vl5lwlz5jbkg4r6p340dwmj6fha3xq-stdenv-linux

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/pjq5v1xgljr4ygb1h1xfybdjydczcqiw-stdenv-linux

@Ericson2314
Copy link
Member Author

Qt 3 part moved to #45888

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/95bw3cdcgc8c0rrvqwvhvc805pdv7rmx-stdenv-darwin

@FRidh FRidh added this to the 18.09 milestone Sep 1, 2018
@timokau
Copy link
Member

timokau commented Sep 1, 2018

Doesn't the title suggest the exact opposite? If we deprecate non-string configureFlags, all configureFlags would be strings.

@dezgeg
Copy link
Contributor

dezgeg commented Sep 1, 2018

especially once we use the Nix 2.0 feature to go directly to bash arrays

Yes, when we get to use the feature this will make sense. But currently we do not, and as the code for using it doesn't exist, it obviously won't make it to 18.09.

And if/when we do it, it doesn't make sense to single out just configureFlags, there are other cases like makeFlags that should be switched out in the same go.

So as such, the situation as in @edolstra's comment seems to still hold: #15799 (comment)

@LnL7
Copy link
Member

LnL7 commented Sep 1, 2018

Yes, when we get to use the feature this will make sense. But currently we do not, and as the code for using it doesn't exist, it obviously won't make it to 18.09.

Isn't something like this necessary as a first step to get nixpkgs ready for structured builder attributes?

(/**/ if lib.isString configureFlags then builtins.trace
"String `configureFlags` is deprecated since 18.09. Please use a list of flags, each a string."
[configureFlags]
else if configureFlags == null then builtins.trace
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure null should be disallowed, since it has a special meaning because of the __ignoreNulls feature in Nix. For example, you can add an attribute like

configureFlags = if stdenv.isDarwin then ["blabla"] else null;

and this won't change the derivation on non-Darwin platforms (so doesn't cause a rebuild).

Copy link
Member Author

Choose a reason for hiding this comment

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

@edolstra Currently because of the way configurePlatforms is handled, it's already a list anyways, so there is no risk of mass rebuilds.

In general, I rather default everything to [], false, etc (or conversely filter out everything that isn't needed) so that the no-rebuild trick happens automatically without needing to write awkward things at the package level.

@dezgeg
Copy link
Contributor

dezgeg commented Sep 1, 2018

Isn't something like this necessary as a first step to get nixpkgs ready for structured builder attributes?

Well, for one we first need a transition plan for not silently breaking code which has list elements
with spaces (i.e. things like [ "--foo 123" "--bar 456" ]) and assume the current behaviour.

@Ericson2314
Copy link
Member Author

@dezgeg We can additionally deprecate spaces unless the Nix 2.0 thing is in use. (I recall being able to use it on a derivation-by-derivation basis.)

If someone tells me how to do that structured attractive check, I'll add it and fix the title (should be "non-list"), and then I think this is ready to go.

@LnL7
Copy link
Member

LnL7 commented Sep 1, 2018

@dezgeg Ah, good point that set of packages is probably much bigger.

At some point it would be nice to remove suppport for this altogether.
@GrahamcOfBorg
Copy link

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/i6vl5lwlz5jbkg4r6p340dwmj6fha3xq-stdenv-linux

@GrahamcOfBorg
Copy link

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/pjq5v1xgljr4ygb1h1xfybdjydczcqiw-stdenv-linux

@GrahamcOfBorg
Copy link

Success on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/95bw3cdcgc8c0rrvqwvhvc805pdv7rmx-stdenv-darwin

@vcunat vcunat changed the title stdenv: Deprecate non-string configureFlags stdenv: Deprecate string configureFlags Sep 2, 2018
@vcunat
Copy link
Member

vcunat commented Sep 2, 2018

I don't feel like putting such a thing into 18.09 this late (especially when a consensus isn't really clear yet). I don't see a problem with postponing this, with master branch (perhaps) getting stricter than stable.

@vcunat vcunat modified the milestones: 18.09, 19.03 Sep 2, 2018
@edolstra
Copy link
Member

edolstra commented Sep 2, 2018

There's not really an advantage to doing this now rather than if/when we ever switch to structured attributes.

@Ericson2314
Copy link
Member Author

@edolstra if we wait until when we actually make the transition, then there's no time to deprecate. The whole point of depreciations is to clear the way for breaking changes that happen later; they must happen in advance to provide any benefit.

@Ericson2314
Copy link
Member Author

@vcunat certainly we should have concensus before doing anything, but I don't think "last-minute" depreciations are bad in and of themselves in that they are purely cautionary: there's no corresponding last minute action the user is forced to take.

The slight downside with a post-release deprecation is we effectively wait another cycle before doing anything. I.e people that only use stable wouldn't see it until 19.03, and I think we all agree that the minimum warning time before some breaking change should be counted for them, not the unstable users that get the head start.

@Ekleog
Copy link
Member

Ekleog commented Jun 2, 2019

(triage) Given there is certainly a need for consensus here, I think this should be made into a proper RFC.

@jtojnar jtojnar mentioned this pull request Jun 5, 2019
10 tasks
@stale
Copy link

stale bot commented Jun 2, 2020

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the
    related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
    irc.freenode.net.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@SuperSandro2000 SuperSandro2000 marked this pull request as draft January 18, 2021 11:46
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 18, 2021
@stale
Copy link

stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 21, 2021
@Artturin
Copy link
Member

Artturin commented May 5, 2022

Let's do this after 22.05 branch off so we do this at some point instead of procrastinating until we have to do this

there's been about 8 releases since 2018 so..

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 5, 2022
@Artturin Artturin closed this May 15, 2022
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.

None yet