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

treewide: Deprecate platform aliases #45717

Closed

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 28, 2018

The first commit is succinct and does the depredations. You can just read it, but they are:

  1. Left in this PR.
    system ==> stdenv.buildPlatform.system
    
  2. Left in this PR.
    stdenv.system ==> stdenv.buildPlatform.system
    
  3. Proposed in other open PR.
    configureFlags = "--foo --bar"; ==> configureFlags = [ "--foo" "--bar" ];
    
  4. Message improved in other PR
    isArm ==> isAarch32 # Already done during 18.03
    
  5. Done in other PR
    buildPlatform ==> stdenv.buildPlatform
    
  6. Done in other PR
    hostPlatform ==> stdenv.hostPlatform
    
  7. Done in other PR
    targetPlatform ==> stdenv.targetPlatform
    

The rest make Nixpkgs itself comply. If this is accepted, sometime after 18.09 is forked I'll merge another PR to actually remove them.

Motivation for this change

See #27069. This makes there be only one non-deprecated way to inspect the build/host/target platform in most cases. stdenv.is* is kept, unlike as in in the issue, since it is the most popular and already a shorthand.

The configureFlags change is unrelated except I earlier did the work of making nixpkgs obey it. It would be nice to deprecate that too so the work doesn't just bit-rot away.

Things done

No hashes should be changed.

  • 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.

@dezgeg
Copy link
Contributor

dezgeg commented Aug 28, 2018

Nah, I don't think there's a need to deprecate anything.

@LnL7
Copy link
Member

LnL7 commented Aug 28, 2018

I'm probably getting confused by build/host/target again, but shouldn't the source tarball conditions use targetPlatform?

@oxij
Copy link
Member

oxij commented Sep 6, 2018

Current state of this PR's changes is confusing:

"... Please use stdenv.system or stdenv.hostPlatform.system."
"stdenv.system is deprecated since 18.09. Please use stdenv.hostPlatform.system."

@Ericson2314
Copy link
Member Author

Hehe that's me hoping the first PR is more palatable than the second. It would be nice to at least get the whole suite of "top-level depreciations" if they both really would like stdenv.system to stay in place.
.system`.

@edolstra
Copy link
Member

edolstra commented Sep 7, 2018

Is this a real use-case?

Yes, this is a real use-case: NixOps has to support multiple NixOS versions. So deprecation warnings are annoying because either we get warnings on newer NixOS versions, or we fix the warnings and break support for the older versions.

@Ericson2314
Copy link
Member Author

What versions of NixOS does NixOPS currently attempt to support? Is this documented anywhere?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 7, 2018

Also, I offer to personally backport stdenv.*Platform.system to prior NixOS versions. It would be wrong for the limited version of cross compilation those offer, but that's fine.

@dezgeg
Copy link
Contributor

dezgeg commented Sep 10, 2018

But you do need to check for the specific architecture from time to time.

That is not OS-specific, so it should be done not with system but with:
stdenv.hostPlatform.isArm && stdenv.hostPlatform.parsed.cpu.version <= 6
That's a totally legit use-case, but not one which I think is common enough to deserve a shorthand.

These system strings are what I and other people know, this other stuff I have no idea even existed. (And given these sort of checks are needed only in ultra rare situations, probably shouldn't since to me they seem to raise more questions than they answer, e.g what's this stdenv.hostPlatform.parsed.cpu.version even gonna return on an x86_64? Will it return "5tel" or 5 on armv5tel-linux?). Now yes that will assume Linux, but that's the only OS supported on ARMv6 anyway and the chances of that changing are literally so low I won't be thinking about that. In fact I don't even know if -latomic is needed on non-GCC-using platforms or is it actually going to break things there.

(Even then, it is bad for platforms with more than one ABI and hostPlatform.config is a better choice).

Same thing, if there is no real practical difference, the interface that people know is better.

I think the main reason to deprecate stdenv.system is the confusion as to what system it is applying to. Is it the system building the software or the system that will run the software. This applies to stdenv.is* as well, though.

Both Buildroot and OpenWRT seem to have e.g. BR2_x86_64 and CONFIG_x86_64 which are exactly equivalent to our stdenv.isx86_64 and there seems to be no confusion.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 10, 2018

These system strings are what I and other people know, this other stuff I have no idea even existed.

So what? The deprecation warning brings awareness to the new interface. It's absurd to argue against advertising the new interface and then complain it's not well known.

Will it return "5tel" or 5 on armv5tel-linux?

5

e.g what's this stdenv.hostPlatform.parsed.cpu.version even gonna return on an x86_64?

The field is only present for ARM, as it's not clear what it should be elsewhere when we don't have multiple versions anyways.

Now yes that will assume Linux, but that's the only OS supported on ARMv6 anyway

iOS. I do use Nix and nixpkgs to build iOS apps for work, so this is not at all hypothetical.

But that to me is besides the point. Except for things like prebuilt binaries, it's usually clear when some change is because of the OS or because of the CPU. If say, we're supporting aarch64-linux, some arm-linux, and x86_64-linux, two orthogonal conditions is much better style than one fat condition on the system. It's crucial to push the interfaces coax developers in the direction of better code.

In fact I don't even know if -latomic is needed on non-GCC-using platforms or is it actually going to break things there.

It is not unlikely one would want to use Clang for these things. compiler-rt provides atomics too, but either library can be used.

Same thing, if there is no real practical difference, the interface that people know is better.

As an aside, I'm not even sure it is a better known interface. Outside of Nix I probably see x86_64-unknown-linux-gnu more than x86_64-linux.

But yes, I do concede that at the moment prebuilt binaries tend do only bother with one ABI per CPU+OS.

@edolstra
Copy link
Member

IMHO, we should really reconsider whether these gratuitous deprecations/removals/renames are a good idea. They all add a little bit of friction to each NixOS upgrade, which, given enough of them, adds up to a lot of friction. Ideally you should be able to try out a newer NixOS version without having to make lots of little changes to your configuration modules / Nix packages, and to use those in multiple NixOS versions without having to put in lots of version conditionals.

@matthewbauer matthewbauer removed this from the 18.09 milestone Nov 5, 2018
@flokli flokli changed the title treewide: Deprecate platform aliases for 18.09 treewide: Deprecate platform aliases Mar 30, 2019
@flokli
Copy link
Contributor

flokli commented Mar 30, 2019

I guess this won't make it into 18.09 or 19.03 ;-)

I updated the PR title. I think we should come to a consensus about deprecations. Could somebody revive RFC 0033?

@Ericson2314
Copy link
Member Author

Yes this is blocked on that RFC, which I do mean to get back to. (I am technically coauthor but haven't done anything yet!! 😲).

@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
@Mic92
Copy link
Member

Mic92 commented Jun 2, 2020

rfc33 was closed. Should we keep this one open?

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020
@flokli
Copy link
Contributor

flokli commented Jun 14, 2020

As stated by @Ericson2314 in #45717 (comment), this is blocked on an RFC, which is yet to be revived.

I propose closing it for now - as the PR doesn't apply cleanly either and is not actionable atm.

@Mic92 Mic92 closed this Jun 14, 2020
@Ericson2314
Copy link
Member Author

Find with me

ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 4, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 4, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 7, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 7, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 8, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 8, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 9, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
ConnorBaker added a commit to ConnorBaker/nixpkgs that referenced this pull request Apr 10, 2024
For more information about *why* this is desirable, see
NixOS#45717
and
NixOS#27069
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: work-in-progress 6.topic: portability General portability concerns, not specific to cross-compilation or a specific platform 6.topic: stdenv Standard environment 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet