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

quartus-prime-lite: Add postInstall option #123469

Closed
wants to merge 1 commit into from

Conversation

Lucus16
Copy link
Contributor

@Lucus16 Lucus16 commented May 18, 2021

Motivation for this change

quartus-prime-lite has a pretty large closure size of about 15GB. This option allows cutting out unneeded parts before the package is stored permanently in the nix store.

For example, if you're only going to use the twentynm platform and you're not going to use the builtin C compiler, you can cut the closure size in half with:

quartus-prime-lite.override {
  postInstall = ''
    rm -r $out/nios2eds/bin/gnu
    find $out/modelsim_ase/altera/{verilog,vhdl}/* ! -name src ! -path '*twentynm*' -delete
  '';
}
Things done

I tested a postInstall similar to the above and was still able to produce fpga images.

  • 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 can be used to remove unused files to reduce closure size.

For example, if you're only going to use the twentynm platform:

    quartus-prime-lite.override {
      postInstall = ''
        find $out/modelsim_ase/altera/{verilog,vhdl}/* ! -name src ! -path '*twentynm*' -delete
      '';
    }
@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 123469 at 3c4c1b9 run on x86_64-linux 1

1 package failed to build:
1 package built successfully:
  • tests.trivial

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@Lucus16
Copy link
Contributor Author

Lucus16 commented May 18, 2021

The robot failed to build because the package is nonfree of course. With the default postInstall, the derivation will not significantly change so there is no risk of introducing new build failures.

@yorickvP yorickvP requested a review from kwohlfahrt May 18, 2021 12:29
Copy link
Contributor

@kwohlfahrt kwohlfahrt left a comment

Choose a reason for hiding this comment

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

Overall, I approve of adding feature flags to reduce closure size, thanks for this work! Is removing folders like this likely to cause crashes at runtime, or does Quartus gracefully handle the case where these folders have been removed?

It might also be worth splitting quartus into quartus and quartus-unwrapped top-level packages, so that people can override the base package more easily.

supportedDevices ? [ "Arria II" "Cyclone V" "Cyclone IV" "Cyclone 10 LP" "MAX II/V" "MAX 10 FPGA" ] }:
{ buildFHSUserEnv, makeDesktopItem, writeScript, stdenv, lib, requireFile, unstick
, supportedDevices ? [ "Arria II" "Cyclone V" "Cyclone IV" "Cyclone 10 LP" "MAX II/V" "MAX 10 FPGA" ]
, postInstall ? ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer some more structured options here. For example, your suggested change could have:

  • disableCCompiler = true
  • platforms = [ "twentynm" ]

I don't know which parts of Quartus can be removed in this way, so I'll leave it to you to design a good interface here.

@Lucus16
Copy link
Contributor Author

Lucus16 commented May 18, 2021 via email

@kwohlfahrt
Copy link
Contributor

Hm, I don't think I know any other packages that expose a postInstall hook in this way. If there are let me know and I'll have a look at them, otherwise this approach seems a bit at odds with how things are done in the rest of nixpkgs.

If we don't have structured options, it might be better to expose the quartus package as quartus-unwrapped or something, so that it's easier to override in different ways. I'll have a look this weekend.

@kwohlfahrt
Copy link
Contributor

I've just opened #124906, which exposes the unrwapped version as a parameter. This means you change could be applied in an overlay, like:

self: super: {
  quartus-prime-lite = super.quartus-prime-lite.override {
    unwrapped = super.quartus-prime-lite.unwrapped.overrideAttrs (o: {
      buildCommand = o.buildCommand + ''
        rm -r $out/nios2eds/bin/gnu
        find $out/modelsim_ase/altera/{verilog,vhdl}/* ! -name src ! -path '*twentynm*' -delete
      '';
    });
  };
}

Would this be sufficient for your use-case? I think that other PR has the approach of being a bit more general, because it allows other attributes of the package to be overridden as well, at the expense of slightly more code.

@Lucus16
Copy link
Contributor Author

Lucus16 commented May 30, 2021 via email

SuperSandro2000 pushed a commit that referenced this pull request Jul 5, 2021
This allows customizing the install process for the unwrapped process,
as proposed in #123469, without introducing top-level support for
untested modifications.

The PR could then be straightforwardly implemented as an overlay, that
does:

  quartus-prime-lite = super.quartus-prime-lite.override {
    unwrapped = quartus-prime-lite.unwrapped.overrideAttrs (o: {
      buildCommand = o.buildCommand + ''
        rm -r $out/nios2eds/bin/gnu
        find $out/modelsim_ase/altera/{verilog,vhdl}/* ! -name src ! -path '*twentynm*' -delete
      '';
    });
  };
@Lucus16 Lucus16 closed this Dec 13, 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

3 participants