Skip to content

writeShellApplication: use shellcheck only where supported#240348

Merged
fgaz merged 1 commit intoNixOS:masterfrom
fgaz:write-shell-application/check-platforms
Aug 9, 2023
Merged

writeShellApplication: use shellcheck only where supported#240348
fgaz merged 1 commit intoNixOS:masterfrom
fgaz:write-shell-application/check-platforms

Conversation

@fgaz
Copy link
Member

@fgaz fgaz commented Jun 28, 2023

Description of changes

GHC (=> shellcheck) isn't supported on some platforms (such as risc-v)
but we still want to use writeShellApplication on those platforms

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@fgaz fgaz force-pushed the write-shell-application/check-platforms branch from 6f4e4ed to d0fe112 Compare June 28, 2023 14:11
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 28, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/2401

@fgaz fgaz mentioned this pull request Jul 7, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it make more sense to tryEval shellcheck itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried, but it returns true, same for non-boot ghc. I think it's because the platform support in the ghc expression is implemented with throw

Copy link
Member

Choose a reason for hiding this comment

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

something like this

Suggested change
shellcheckSupported = (builtins.tryEval ghc.bootPkgs.ghc).success;
shellcheckSupported = lib.meta.availableOn stdenv.hostPlatform ghc;

or

Suggested change
shellcheckSupported = (builtins.tryEval ghc.bootPkgs.ghc).success;
shellcheckSupported = lib.meta.availableOn stdenv.hostPlatform shellcheck;

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't you mean buildPlatform? Also this evaluates to true. I think it's because the platform support in the ghc expression is implemented with throw

Copy link
Member

Choose a reason for hiding this comment

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

Can we convince the ghc people to change that to meta.platforms? Maybe it is super simple because they generate many things.

Copy link
Member

Choose a reason for hiding this comment

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

@NixOS/haskell

Copy link
Member

@sternenseemann sternenseemann Aug 2, 2023

Choose a reason for hiding this comment

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

All the checks proposed here are (slightly) incorrect. That using meta.platforms is not possible is a slight oversight, namely that (shallowly) evaluating passthru (which mkDerivation needs to do) forces binDistUsed. I'll correct that separately and then it should be possible again to use meta.platforms.

The correct check for shellcheck's availability is lib.meta.availableOn stdenv.buildPlatform shellcheck.compiler which checks if the compiler we would use to build shellcheck itself is available. I need to warn you though that meta.platforms is imperfect, so there may be some edge cases, but I'm not sure if this applies to any platforms stdenv supports as a build platform. This is improved in #212188.

Edit: #246805

@fgaz fgaz force-pushed the write-shell-application/check-platforms branch from d0fe112 to fed2c13 Compare July 19, 2023 10:44
@fgaz
Copy link
Member Author

fgaz commented Jul 24, 2023

Ping

1 similar comment
@fgaz

This comment was marked as duplicate.

@fgaz fgaz force-pushed the write-shell-application/check-platforms branch from fed2c13 to 2c5990f Compare August 5, 2023 16:31
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Aug 5, 2023
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Note that the current approach will stop working after #243619. Seems like we will need to invent a completely new approach for this sort of thing then (i.e. some way to check if GHC can be bootstrapped in the context of the current package set(s)).

@fgaz fgaz merged commit b2e987d into NixOS:master Aug 9, 2023
@fgaz fgaz deleted the write-shell-application/check-platforms branch August 9, 2023 10:17
@sternenseemann
Copy link
Member

#339272 will break this. Probably no other option than using tryEval then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants