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/parse: adjust isCompatible description #113982

Merged
merged 1 commit into from May 14, 2021

Conversation

siraben
Copy link
Member

@siraben siraben commented Feb 22, 2021

Motivation for this change

Converting the isCompatible definition into a graph shows that CPUs with multiple modes of endianness are indeed isomorphic.

cpu_compat dot

Stating that CPUs and the isCompatible relation forms a category (or preorder) is correct but overtly technical. We can state it more clearly for readers unfamiliar with mathematics while retaining some keywords to be useful to technical readers.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Stating that CPUs and the isCompatible relation forms a category (or
preorder) is correct but overtly technical.  We can state it more
clearly for readers unfamiliar with mathematics while retaining some
keywords to be useful to technical readers.
@siraben
Copy link
Member Author

siraben commented Feb 26, 2021

@Ericson2314 could you take a look?

Copy link
Contributor

@r-burns r-burns left a comment

Choose a reason for hiding this comment

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

As a reader unfamiliar with mathematics, I can attest that this description is much more understandable ;)

You may be interested in NixOS/nix#2595, which touches on the nuance of platform compatibility. It can in some cases depend on more specific details of the OS or processor.

@siraben
Copy link
Member Author

siraben commented Mar 14, 2021

@Ericson2314 good to merge?

@siraben
Copy link
Member Author

siraben commented May 13, 2021

pinging @jonringer

@rmcgibbo
Copy link
Contributor

As a reader unfamiliar with mathematics, I can attest that this description is much more understandable ;)

This is exactly my sentiment as well. 👍

@Ericson2314
Copy link
Member

I agree "category" is over kill, but i rather call it a "partial order" than mere relation. I think "partial orders" (or "preorders) are pedestrian enough, and the vast majority of the relations that arise during packaging are partial orders, so that's the most useful concept to nudge at.

Otherwise looks good; those are partial order axioms (and we might link wikipedia) but it's fine to include them.

@siraben
Copy link
Member Author

siraben commented May 13, 2021

I agree "category" is over kill, but i rather call it a "partial order" than mere relation. I think "partial orders" (or "preorders) are pedestrian enough, and the vast majority of the relations that arise during packaging are partial orders, so that's the most useful concept to nudge at.

Ok. I was hesistant to add partial order because it might not be familiar to many programmers. I can find a way to fit it in somewhere.

@jonringer
Copy link
Contributor

I think "partial orders" (or "preorders) are pedestrian enough

I think this is only pedestrian in circles like haskell, which has a very large mathematical influence. I'm pretty sure if I asked my co-workers over the past 6 years, I would be very hard pressed to find someone which definitively knew what that was.

Either way, I think @siraben 's wording is much more clearer, and the examples help make it much more intuitive.

@Ericson2314
Copy link
Member

@jonringer agreed is an obscure math thing, but aren't relations also obscure? I would have thought they are about as obscure so partial order is no worse in that record.

Anyways I don't want to be the one bikeshedding this to death. It is an improvement already I agree.

@siraben
Copy link
Member Author

siraben commented May 14, 2021

I assume most Nix users are aware of the concept of a relation (transitive closure is already an integral concept in the Nix store at least).

@mohe2015
Copy link
Contributor

I assume most Nix users are aware of the concept of a relation (transitive closure is already an integral concept in the Nix store at least).

I disagree. In Germany you usually learn about relations in university. And also I knew what is meant by a transitive closure but got it to know under another name (recursive dependencies or so)

@mohe2015
Copy link
Contributor

I think we should be as inclusive as possible and if it's possible to document this otherwise (or both ways) I think we should do that.

@mohe2015
Copy link
Contributor

I checked the diff and I think it's fine though

@siraben
Copy link
Member Author

siraben commented May 14, 2021

I disagree. In Germany you usually learn about relations in university. And also I knew what is meant by a transitive closure but got it to know under another name (recursive dependencies or so)

Fair enough, so I mentioned relations but also gave plain English explanations of the desired properties of isCompatible. I'm going to leave it as is, seeing that everyone seem to agree it's an improvement.

@jonringer jonringer merged commit f986333 into NixOS:master May 14, 2021
@jonringer
Copy link
Contributor

otherwise (or both ways)

I would also be fine with having it extended. It's unfortunate that many mathematical concepts have many names depending on what specialty it derived from.

@siraben siraben deleted the parse-patch1 branch May 15, 2021 01:57
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

6 participants