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

vimPlugins.vim-colorschemes: add overlay to address hash issue #157622

Merged
merged 1 commit into from
Mar 18, 2022

Conversation

malob
Copy link
Member

@malob malob commented Feb 1, 2022

Motivation for this change

As discussed in #157609, the source for vim-colorschemes contains two files with who's name only differ by the case:

  • colors/darkBlue.vim
  • colors/darkblue.vim

This results in update.py generating a different hash for the plugin depending on whether the file system of the system it's run on is case-sensitive or not. This mainly comes up on Darwin systems.

There's an issue on the upstream repo (flazz/vim-colorschemes#186) as well as a PR to fix the issue (flazz/vim-colorschemes#181).

Unfortunately the owner of the repo hasn't been active for over a year, so waiting for the PR to land probably isn't a viable solution :(

This PR is a proposed hack to address the issue.

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/)
  • 22.05 Release Notes (or backporting 21.11 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.

@malob
Copy link
Member Author

malob commented Feb 1, 2022

Result of nixpkgs-review pr 157622 run on aarch64-darwin 1

1 package built:
  • spacevim

@malob
Copy link
Member Author

malob commented Feb 1, 2022

Result of nixpkgs-review pr 157622 run on x86_64-linux 1

1 package built:
  • spacevim

@malob
Copy link
Member Author

malob commented Mar 14, 2022

Result of nixpkgs-review pr 157622 run on aarch64-darwin 1

1 package built:
  • spacevim

@malob
Copy link
Member Author

malob commented Mar 14, 2022

Result of nixpkgs-review pr 157622 run on x86_64-linux 1

1 package built:
  • spacevim

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

I think the hash would need to be recomputed as well

# https://github.com/NixOS/nixpkgs/issues/157609
vim-colorschemes = super.vim-colorschemes.overrideAttrs (old: {
src = old.src.overrideAttrs (srcOld: {
postFetch = srcOld.postFetch + lib.optionalString (!stdenv.isDarwin) ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
postFetch = srcOld.postFetch + lib.optionalString (!stdenv.isDarwin) ''
postFetch = (srcOld.postFetch or "") + lib.optionalString (!stdenv.isDarwin) ''

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonringer, I updated the PR with the change you suggested.

I initially thought the same thing about the hash, however, the current hash in nixpkgs is based off the source being downloaded on a darwin system. Given the override does nothing on darwin systems, the hash is the same. When I was testing this to ensure the hashes were the same on both systems I set the sha265 hash for the vim-colorschemes source to "" in generated.nix, to see what hashes Nix expected on each system.

On my Mac:

error: hash mismatch in fixed-output derivation '/nix/store/6yy77dsf8j3dmsia15w1xmbmxh2w1742-source.drv':
         specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
            got:    sha256-5Y29LUC1cddrX/mwLQoMTWxRcvXHYrgRo+s5IKRx+k4=

On my NixOS machine:

error: hash mismatch in fixed-output derivation '/nix/store/7hzr62y9l08j1c37flskx890p0h489dk-source.drv':
         specified: sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
            got:    sha256-5Y29LUC1cddrX/mwLQoMTWxRcvXHYrgRo+s5IKRx+k4=

The expected hash is the same as the current hash in generated.nix:

nix hash to-sri --type sha256 0kpsf6j20fgblc8vhqn7ymr52v2d1h52vc7rbxmxfwdm80nvv3g5
sha256-5Y29LUC1cddrX/mwLQoMTWxRcvXHYrgRo+s5IKRx+k4=

Copy link
Member

Choose a reason for hiding this comment

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

The or "" is only added to prevent evaluation errors if the overwriten Derivation for some reason no longer contain postFetch.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I assumed yeah.

@malob malob force-pushed the fix-vim-colorschemes-hash branch from 1346094 to b348e05 Compare March 15, 2022 17:52
@malob malob requested a review from jonringer March 15, 2022 18:22
@malob malob force-pushed the fix-vim-colorschemes-hash branch from b348e05 to c332788 Compare March 15, 2022 18:23
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

LGTM

Result of nixpkgs-review pr 157622 run on x86_64-linux 1

2 packages built:
  • spacevim
  • tests.vim.test_vim_with_vim_nix

@jonringer jonringer merged commit 08e25bd into NixOS:master Mar 18, 2022
@malob malob deleted the fix-vim-colorschemes-hash branch March 18, 2022 19:58
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.

3 participants