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

haskell.compiler.*: Preserve versions #109815

Closed
wants to merge 2 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 18, 2021

Motivation for this change

This PR illustrates the refactoring I had in mind for #109256.
I'd like to know whether it aligns with @peti expectations.
I'm writing this in my limited free time hoping that it helps someone. Feel free to take this commit and complete it, because it's unlikely that I will return to it.

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.

This generates two sets of attr names in haskell.compiler, from
the same source. By doing so, we can move toward a nicer naming
scheme and we can create aliases for the major releases.
@peti
Copy link
Member

peti commented Jan 20, 2021

Wow, this change is WAY more complicated than I expected. I thought you would add 3 lines that alias ghc86x to ghc865, etc. I did not expect that you would rewrite almost the entire file! Is there any reason why you did not choose the simple solution for providing these aliases?

@roberth
Copy link
Member Author

roberth commented Jan 20, 2021

Is there any reason why you did not choose the simple solution for providing these aliases?

@peti By refactoring it, I figured we can address:

However, a "safety" mechanism must be introduced in nixpkgs in order to ensure that the alias stays in sync.

from the issue.

Without refactoring it, we don't have the data to compute the latest ghc minor version.

The package sets can also be generated from the list, by adding attributes for the compiler config and optional package set config.

I like that it simplifies the integer-simple / native-bignum logic and that it removes the risk of a change accidentally causing an infinitely big tree of recurseIntoAttrs sets.
When the package sets are refactored, the boilerplate will be reduced, reducing copy paste errors.

Furthermore, this approach opens the door to migrate the naming convention with little effort. Doing so could prevent a problem like what go encountered.

So yes, it will not be a simple solution to the issue I referenced, but I do believe that it simplifies the code and makes it more robust and useful. (when done)

Do you think these are a valid changes?

@peti
Copy link
Member

peti commented Jan 22, 2021

To be honest, I never thought that keeping the aliases in sync with the proper "latest version" is going to be a problem. Yes, it's a manual process that can go wrong, but I don't think it's likely. And even if it does, then it's pretty easy to detect and to fix. I would not have bother rewriting the entire file; I would have just added three lines for the compiler aliases and probably three more lines to add package set aliases (that re-use the compiler aliases). I kinda prefer simple solutions. That code you've come up with is probably really great, but it's not like many people will be able to read and understand it, which is a bit of a problem because we lock out potential future maintainers from contributing.

@roberth
Copy link
Member Author

roberth commented Jan 22, 2021

Sure. I agree simple is good, which is why I asked for review before completing this and I'm glad I did.

@roberth roberth closed this Jan 22, 2021
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

2 participants