Skip to content

shellcheck: add override for newer version#147780

Merged
sternenseemann merged 1 commit intoNixOS:haskell-updatesfrom
qowoz:shellcheck
Dec 1, 2021
Merged

shellcheck: add override for newer version#147780
sternenseemann merged 1 commit intoNixOS:haskell-updatesfrom
qowoz:shellcheck

Conversation

@zowoq
Copy link
Contributor

@zowoq zowoq commented Nov 28, 2021

Motivation for this change
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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@zowoq zowoq marked this pull request as draft November 28, 2021 22:27
@github-actions github-actions bot added the 6.topic: haskell General-purpose, statically typed, purely functional programming language label Nov 28, 2021
@zowoq zowoq mentioned this pull request Nov 28, 2021
Merged
6 tasks
zowoq
zowoq commented Nov 28, 2021
View reviewed changes
pkgs/development/haskell-modules/hackage-packages.nix Outdated
Copy link
Contributor Author

@zowoq zowoq Nov 28, 2021

Choose a reason for hiding this comment

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

I did this manually, setting doDistribute and regenerating didn't seem to work.

Copy link
Member

@sternenseemann sternenseemann Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

doDistribute doesn't affect the source, just the evaluation result, what exactly did not work? Changes in hackage-packages.nix will be rolled back.

Copy link
Contributor Author

@zowoq zowoq Nov 29, 2021

Choose a reason for hiding this comment

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

doDistribute doesn't affect the source, just the evaluation result, what exactly did not work?

I thought doDistribute would remove hydraPlatforms = lib.platforms.none; but setting it and regenerating didn't produce any changes.

Changes in hackage-packages.nix will be rolled back.

Yes, I know. What is the correct way to remove this line?

Copy link
Member

@sternenseemann sternenseemann Nov 30, 2021

Choose a reason for hiding this comment

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

Yes, I know. What is the correct way to remove this line?

I think they are non-configurable for the versioned attributes, but doDistribute would achieve the same effect after evaluation. In any case populating hydraPlatforms for this is not strictly necessary, since it is not poisonous: If shellcheck has hydraPlatforms != [] (or unspecified), shellcheck and all its dependencies (including ShellCheck_0_8_0) will be built regardless of their individual hydraPlatforms lists.

Copy link
Contributor Author

@zowoq zowoq Nov 30, 2021

Choose a reason for hiding this comment

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

shellcheck inherits meta from ShellCheck_0_8_0 which will include the disabled hydraPlatforms.

Copy link
Contributor Author

@zowoq zowoq Nov 30, 2021

Choose a reason for hiding this comment

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

Pushed doDistribute for ShellCheck_0_8_0.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Nov 28, 2021
sternenseemann
sternenseemann reviewed Nov 30, 2021
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
Copy link
Member

@sternenseemann sternenseemann Nov 30, 2021

Choose a reason for hiding this comment

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

I would not recommend overriding in this way, as it is brittle at best because it bypasses the haskellPackages fixpoint.

Copy link
Contributor Author

@zowoq zowoq Nov 30, 2021

Choose a reason for hiding this comment

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

Can you suggest an alternative?

Copy link
Member

@sternenseemann sternenseemann Nov 30, 2021

Choose a reason for hiding this comment

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

Suggested change
haskellPackages.ShellCheck = haskellPackages.ShellCheck_0_8_0;
ShellCheck = haskellPackages.ShellCheck_0_8_0;

Then take ShellCheck as an input instead of haskellPackages in shellcheck/default.nix. It is the only attribute used anyways.

Copy link
Contributor Author

@zowoq zowoq Nov 30, 2021

Choose a reason for hiding this comment

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

Updated.

Copy link
Member

@sternenseemann sternenseemann Nov 30, 2021

Choose a reason for hiding this comment

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

Seems alright, the shellcheck expression could probably use a refactor, but that can be done another time (by someone else).

Copy link
Contributor Author

@zowoq zowoq Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What do you have in mind for a refactor? I'll try to find some time in the next few weeks.

@sternenseemann
Copy link
Member

sternenseemann commented Nov 30, 2021

@ofborg build shellcheck

@ofborg ofborg bot added the 8.has: clean-up This PR removes packages or removes other cruft label Nov 30, 2021
@ofborg ofborg bot requested a review from Profpatsch November 30, 2021 01:03
@zowoq
Copy link
Contributor Author

zowoq commented Nov 30, 2021

cleanup label was added because doDistribute doesn't include all of the platforms that were set previously e.g. aarch64-linux.

nixpkgs/pkgs/development/haskell-modules/lib/compose.nix

Line 124 in 14d0efe

doDistribute = overrideCabal (drv: { hydraPlatforms = drv.platforms or ["i686-linux" "x86_64-linux" "x86_64-darwin"]; });

Not sure what to do about that?

@sternenseemann
Copy link
Member

sternenseemann commented Nov 30, 2021

I think it would probably be easiest to do do meta = builtins.removeAttrs ShellCheck.meta [ "hydraPlatforms" ] in the shellcheck derivation for now.

@zowoq
Copy link
Contributor Author

zowoq commented Nov 30, 2021

@ofborg build nix-info-tested

@zowoq zowoq marked this pull request as ready for review November 30, 2021 02:57
@zowoq zowoq changed the base branch from master to haskell-updates December 1, 2021 22:09
@zowoq
shellcheck: add override for newer version
9326641 
`haskellPackages.ShellCheck` is pinned on stackage.
@zowoq
Copy link
Contributor Author

zowoq commented Dec 1, 2021

Just realised this isn't already on haskell-updates so I've rebased onto that branch. I'll merge in a few hours.

@sternenseemann
Copy link
Member

sternenseemann commented Dec 1, 2021

@ofborg build shellcheck

@sternenseemann sternenseemann merged commit 0ce7784 into NixOS:haskell-updates Dec 1, 2021
@zowoq
Copy link
Contributor Author

zowoq commented Dec 1, 2021 •
edited
Loading

Thanks for your help with this @sternenseemann!

@zowoq zowoq deleted the shellcheck branch December 1, 2021 23:14
@zowoq zowoq mentioned this pull request Dec 26, 2022
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sternenseemann sternenseemann sternenseemann approved these changes

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@expipiplus1 expipiplus1 Awaiting requested review from expipiplus1

@maralorn maralorn Awaiting requested review from maralorn

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

Assignees

No one assigned

Labels

6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zowoq @sternenseemann