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

kanata: fix hash mismatch on case-insensitive filesystems #334347

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

jian-lin
Copy link
Contributor

Description of changes

related #308089

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@jian-lin
Copy link
Contributor Author

CC @toonn for review since this is suggested by you.
CC @thefloweringash for review since the duplicated files are found by you. BTW, could you share the way you use to find that?

@SuperSandro2000
Copy link
Member

Could you also send an upstream PR to resolve this?

@jian-lin
Copy link
Contributor Author

Which upstream do you mean?

Presumably, the issue is in cargo. I just linked two cargo issues in #308089 (comment). cargo maintainers think they have not come to a conclusion on if it should be fixed and what the right constraints are. So I will not fix this in cargo. Fixing this in Nixpkgs eases the burden of bumping to new version a lot.

Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

LGTM

Does upstream really intend for there to be two differently capitalized READMEs though, seems quirky?

@jian-lin
Copy link
Contributor Author

Does upstream really intend for there to be two differently capitalized READMEs though

Looks like so.


Merge conflicts are resolved. I have verified that the hash is the same on both linux and darwin. Let's merge this.

@jian-lin jian-lin merged commit e4ca168 into NixOS:master Aug 14, 2024
8 of 9 checks passed
@jian-lin jian-lin deleted the pr/kanata-unify-hash branch August 14, 2024 14:48
@toonn
Copy link
Contributor

toonn commented Aug 15, 2024

Where is README.md coming from actually? The native-windows-gui repo seems to only have readme.mds, though 1.0.13, the version on crates.io, is not a tag in the repo.

I also checked Cargo.tomls, one of the issues mentioned problems when those contained a different case for README files but none of them seem to mention any.

@jian-lin
Copy link
Contributor Author

Does upstream really intend for there to be two differently capitalized READMEs
Where is README.md coming from actually? The native-windows-gui repo seems to only have readme.mds, though 1.0.13, the version on crates.io, is not a tag in the repo.

Oh, so by "upstream" you mean native-windows-gui? I thought you meat cargo by "upstream" because the real issue is in cargo.

My understanding is that native-windows-gui's readme file is named readme.md, but cargo wants a README.md, so cargo generates a README.md from readme.md when publishing a crate. That is exactly rust-lang/cargo#14020. You can verify this following this command on a case-sensitive FS.

@jian-lin
Copy link
Contributor Author

The subtle thing about reproducing steps in rust-lang/cargo#14020 is that you have to run cargo package on a case-insensitive FS!

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