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

lib/systems: rename examples.nix to all.nix, include alias #175341

Closed
wants to merge 2 commits into from
Closed

lib/systems: rename examples.nix to all.nix, include alias #175341

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 29, 2022

Background

When I had to figure out how lib/systems worked in order to enable bootstrapping on mips64el and powerpc64le, by far the most confusing thing was that nixpkgs' official list of platforms (i.e. those identifiers which can be used in pkgsCross.<platform>) is kept in a file called examples.nix. When most people, including myself, see a codebase with files or directories named examples/ they assume it contains examples which are for tutorial purposes only, and are not used by the deployed codebase.

This commit attempts to save future nixpkgs contributors the confusion I encountered.

Description of changes

This commit renames lib/systems/examples.nix to lib/systems/all.nix, since what it contains is a list of all systems (or platforms) that nixpkgs currently knows about. This commit also includes an alias in lib/systems/examples.nix, so it is not a breaking change.

Future Changes

If this commit is merged I will submit separate PRs to update all the references to lib.systems.examples in nixpkgs so they point to lib.systems.all instead.

At some point in the future I would like to gradually migrate towards removing the alias, in steps:

  1. merge this commit
  2. replace the examples.nix alias with a trace
  3. replace the trace with a throw
  4. drop examples.nix entirely

...with no more than one step taken in each twice-per-year release.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

When I had to figure out how `lib/systems` worked in order to enable
bootstrapping on mips64el and powerpc64el, by far the most confusing
thing was that nixpkgs' official list of platforms (i.e. those
identifiers which can be used in `pkgsCross.<platform>`) is kept in a
file called `examples.nix`.  When most people, including myself, see a
codebase with files or directories named `examples/` they assume it
contains *examples* which are for tutorial purposes only, and are not
used by the deployed codebase.

This commit attempts to save future nixpkgs contributors the confusion
I encountered.  It renames `lib/systems/examples.nix` to
`lib/systems/all.nix`, since what it contains is *a list of all
systems (or platforms) that nixpkgs currently knows about*.  This
commit also includes an alias in `lib/systems/examples.nix`, so it is
not a breaking change.
Copy link
Member

@ckiee ckiee left a comment

Choose a reason for hiding this comment

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

Yes please! This was pretty annoying the first time I saw it

Future Changes

If this commit is merged I will submit separate PRs to update all the references to lib.systems.examples in nixpkgs so they point to lib.systems.all instead.

Why plural "PRs"? Better to do it in one swoop to avoid more new references being added.

At some point in the future I would like to gradually migrate towards removing the alias, in steps: merge this commit, then replace the examples.nix alias with a throw, then drop examples.nix entirely -- with no more than one step taken in each twice-per-year release.

Did you mean trace instead of throw? That'd seem more reasonable and would also let us know if there's anything blocking removal. (think Nix uses this somewhere)

@@ -0,0 +1,336 @@
# These can be passed to nixpkgs as either the `localSystem` or
Copy link
Member

Choose a reason for hiding this comment

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

You need to add this to lib/systems/default.nix (here). It doesn't seem to do any magic

Copy link
Author

@ghost ghost May 30, 2022

Choose a reason for hiding this comment

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

Why plural "PRs"? Better to do it in one swoop to avoid more new references being added.

Okay!

Did you mean trace instead of throw?

Ah, I was wondering how to have nixpkgs print a "warning". I assumed trace was supposed to be for debugging only. If it is okay to commit expressions that use it to print warnings, then I would like to add that as an additional step:

At some point in the future I would like to gradually migrate towards removing the alias, in steps:

  1. merge this commit
  2. replace the examples.nix alias with a trace
  3. replace the trace with a throw
  4. drop examples.nix entirely

...with no more than one step taken in each twice-per-year release.

I have edited the topmost comment to reflect this.

Copy link
Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

I meant:

  all = import ./all.nix { inherit lib; };
  examples = import ./examples.nix { inherit lib; };

We can't change the file structure yet since people might be importing lib/systems/examples.nix directly for whatever reason, so this file should just have a 1-1 mapping of the directory structure.

@ghost ghost requested a review from ckiee May 30, 2022 17:56
@Ericson2314
Copy link
Member

I don't like renaming it, because they really are just examples. Anything that parse.nix will parse is fair game. Shame on pkgsCross for making people think otherwise.

@ghost
Copy link
Author

ghost commented May 30, 2022

Anything that parse.nix will parse is fair game.

To clarify, for those following along: parse.nix's mkSystemFromString accepts the "multiarch" platform tuples you're used to seeing from GCC and autoconf, like x86_64-unknown-linux-gnuabix32 (but with very-forgiving parsing, probably loose enough to accept nix's 2-tuples as well).

I don't like renaming it, because they really are just examples.

I don't like the fact that pkgsCross has turned lib/systems/examples.nix into a new platform taxonomy either. It drives me crazy how many projects invent their own platform taxonomies, and how many Rosetta Stones we need because of this.

Instead of this PR, I would be happy to see us migrate towards forcing pkgsCross to use standard tuples, in steps, no more than one per biannual release:

  1. Emit a trace warning when people use pkgsCross.foo where mkSystemFromString rejects foo or else returns something not equal to (import examples.nix).foo
  2. throw when the above happens
  3. Drop the code that checks examples.nix when resolving pkgsCross.foo

@Ericson2314, would you support such an effort, including acting as shepherd for an RFC if one is deemed necessary? I would prefer that instead of this PR. But the current ambiguity is much worse than either of those solutions.

I've marked this as draft so nobody merges it before we hear back from you.

@ghost ghost marked this pull request as draft May 30, 2022 18:49
@Ericson2314
Copy link
Member

@a-m-joseph I would support that. In fact, I would support just getting rid of pkgCross. I never liked it; the proper thing to do is invoke Nixpkgs with the crossSystem one wants.

@ckiee
Copy link
Member

ckiee commented May 31, 2022

[...] In fact, I would support just getting rid of pkgCross. I never liked it; the proper thing to do is invoke Nixpkgs with the crossSystem one wants.

So how will you do this? pkgsCross is a very convenient little shortcut and is way less verbose than import pkgs.path { crossSystem = blah; }, not to mention that that's probably gonna be a lot more of nixpkgs evals which will probably be crappy for performance. (pkgsCross.whatever.hello can reuse the same nixpkgs evaluation, while (import pkgs.path { crossSystem = "whatever"; }).hello not so much)

@ckiee ckiee removed their request for review May 31, 2022 12:04
@@ -6,7 +6,7 @@ rec {
parse = import ./parse.nix { inherit lib; };
inspect = import ./inspect.nix { inherit lib; };
platforms = import ./platforms.nix { inherit lib; };
examples = import ./examples.nix { inherit lib; };
examples = import ./all.nix { inherit lib; };
Copy link
Member

Choose a reason for hiding this comment

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

This is a big no, as explained by @Ericson2314.
We can tolerate a vague name sometimes, but we can not have factually incorrect ones.

@roberth
Copy link
Member

roberth commented May 31, 2022

reuse the same nixpkgs evaluation

This seems like a valid requirement. Maybe talk to Eelco about merging builtins.memoise? The alternative is actually enumerating all configs in nested attrsets and having a helper query those before returning the nixpkgs invocation. That would complicate the code quite a bit, unlike memoise.

@ghost
Copy link
Author

ghost commented May 31, 2022

pkgsCross is a very convenient little shortcut and is way less verbose than import pkgs.path { crossSystem = blah; }

If I have understood correctly, the idea is pkgsCross.foo where mkSystemFromString accepts foo. For example, pkgsCross.aarch64-linux.

Edit, in case this resolves confusion: the valid arguments to mkSystemFromString can be enumerated. Writing an enumerator for them will be the first implementation step.

(pkgsCross.whatever.hello can reuse the same nixpkgs evaluation, while (import pkgs.path { crossSystem = "whatever"; }).hello not so much)

It will still be pkgsCross.whatever; the change is in how whatever is interpreted.

@ghost
Copy link
Author

ghost commented May 31, 2022

Closing in favor of this approach, which I much prefer. It will be about two weeks before I can start drafting an early-revision proposal, though -- have a few other things ahead of it in the to-do queue.

@ghost ghost closed this May 31, 2022
@ghost ghost deleted the pr/platforms/rename-examples branch January 23, 2024 06:49
This pull request was closed.
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.

3 participants