Skip to content

Conversation

@SuperSandro2000
Copy link
Member

Description of changes
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.

@ofborg ofborg bot requested review from globin, primeos and wmertens May 8, 2022 21:53
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels May 8, 2022
@SuperSandro2000
Copy link
Member Author

@ofborg build git

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

IMO we should figure out a better approach for this. There are quite a few contributed tools (https://github.com/git/git/tree/master/contrib) and they shouldn't really be part of the official Git package. Might be best to add a git-contrib package (either with all tools in one package or as an attrset and packages like git-contrib.git-jump).

Edit: Looks like we're already shipping quite a few of the contributed tools. That isn't ideal to begin with... :o

@SuperSandro2000
Copy link
Member Author

We also install quite a few contrib things under $out/share/git/contrib/ already and not having them in the PATH is inconvenient. I don't think this PR should be blocked by this and the contrib directory is only 1.1MB in size.

@primeos
Copy link
Member

primeos commented May 14, 2022

We also install quite a few contrib things under $out/share/git/contrib/ already

Yes, I also noticed this, and IMO it's wrong. I'd prefer if we'd stop doing that.

not having them in the PATH is inconvenient

Sure but it's also the point. Those tools aren't part of a normal installation and it isn't that inconvenient to add them to $PATH, if necessary.

the contrib directory is only 1.1MB in size.

It isn't about the size.

Just to be sure I checked what two other Linux distributions are doing:

@SuperSandro2000
Copy link
Member Author

IMO it's wrong. I'd prefer if we'd stop doing that.

Whats wrong about this? If we have the files hidden deeply in share we can also add a symlink to bin. Not like its going to break something.

Those tools aren't part of a normal installation and it isn't that inconvenient to add them to $PATH, if necessary.

The completion script is also not part of the standard installation and installed by many distributions from contrib.


Can we please move this discussion to another place? It is unlikely that we are going to remove everything from contrib in this PR.

@primeos
Copy link
Member

primeos commented May 16, 2022

Whats wrong about this?

Unfortunately I didn't find good sources regarding the contrib/ directory conventions / best practices but AFAIK it just doesn't belong into the same package as the content is contributed by the community and not an "official" part of the software (and it's usually also not maintained by upstream). IMO https://drewdevault.com/2020/06/06/Add-a-contrib-directory.html explains it nicely though (but one may or may not like it). https://9p.io/wiki/plan9/Contrib/index.html is also interesting.

The completion script is also not part of the standard installation and installed by many distributions from contrib.

Yes, that does indeed seem to be an exception. Ideally, it wouldn't be in the contrib/ directory anymore.

Can we please move this discussion to another place?

Sure, feel free to open a dedicated issue to discuss this matter (and please reference it here) ;) That'd be great :)

It is unlikely that we are going to remove everything from contrib in this PR.

That's fine - I'd just like to avoid making the situation worse than it already is (IMO - that's why a dedicated issue to discuss it in our community would be good).

@SuperSandro2000
Copy link
Member Author

Can we please move this discussion to another place?

I was more thinking about merging this PR and then someone that is not me and actually cares about the matter opens an issue and can discuss that with other people.

@SuperSandro2000
Copy link
Member Author

@ofborg eval


I am planning to merge this in the next days. Another symlink is not going to make a difference and I am currently not planning to majorly refactor the git package.

@ofborg ofborg bot requested a review from primeos October 5, 2022 15:39
@SuperSandro2000 SuperSandro2000 merged commit e0d4d69 into NixOS:staging Oct 14, 2022
@SuperSandro2000 SuperSandro2000 deleted the git-jump branch October 14, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants