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

Jump to bottom

gitAndTools.gitin: init at 0.27.8 #86070

Merged
merged 4 commits into from May 24, 2020
Merged

gitAndTools.gitin: init at 0.27.8 #86070

merged 4 commits into from May 24, 2020

Conversation

kimat
Copy link
Contributor

@kimat kimat commented Apr 26, 2020

Motivation for this change

Make it possible to install gitin using nix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

doronbehar
doronbehar requested changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

As for the commit messages, you should eventually have:

maintainers: add kimat
gitin: init at 0.2.3

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/applications/version-management/git-and-tools/gitin/default.nix Outdated Show resolved Hide resolved
pkgs/applications/version-management/git-and-tools/gitin/default.nix Outdated Show resolved Hide resolved
@kimat kimat force-pushed the kimat-gitin branch 3 times, most recently from 2948c10 to cb395cc Compare April 28, 2020 17:36
doronbehar
doronbehar reviewed Apr 28, 2020
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -3687,6 +3687,8 @@ in

gitfs = callPackage ../tools/filesystems/gitfs { };

gitin = callPackage gitAndTools.gitin;
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
gitin = callPackage gitAndTools.gitin;
gitin = gitAndTools.gitin;

Otherwise, nix-build -A gitin fails with:

error: expression does not evaluate to a derivation (or a set or list of those)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added this change too.

I suppose adding to this line is deprecated to keep diffs easier :
https://github.com/NixOS/nixpkgs/blob/b3b8f66ecc49e9592ccba041519ed9afac9aa355/pkgs/top-level/all-packages.nix#L19666

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. Go for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides that all looks good.

Copy link
Contributor Author

@kimat kimat Apr 29, 2020 •
edited

Choose a reason for hiding this comment

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

Actually, I was trying to say one should not add it there.

It looks like this was an old convention and all the packages on that line were added in 2015 or before.

After some more research on my side, I think I uncovered that recent packages aren't added to all-packages.nix at all:

Should I just remove my changes in all-packages.nix and rename this merge request to:
gitAndTools.gitin: init at 0.27.8 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doronbehar should I go for

  1. gitin = gitAndTools.gitin; (current version)
  2. inherit (gitAndTools) git ... gitin
  3. don't add it to all-packages.nix and have it only usable with the full name : gitAndTools.gitin

I think it should be option 1. or 3.

I was trying to say in this comment : #86070 (comment), that 2. should be avoided since :

  • it makes diffs harder
  • recent gitAndTools packages have not been added here

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say go for 3, since that seems to be current practice for new packages - we can always add it to all-packages.nix later if that feels necessary. It will still appear in searches, so we're not losing much discoverability either. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talyz, yes, completely. I just updated the PR with those changes.

@kimat kimat force-pushed the kimat-gitin branch 2 times, most recently from 2656fe0 to b3b8f66 Compare April 28, 2020 19:00
jtojnar
jtojnar reviewed Apr 29, 2020
View reviewed changes
pkgs/top-level/all-packages.nix Outdated
@@ -11750,6 +11752,16 @@ in
inherit (darwin.apple_sdk.frameworks) Security;
};

libgit2_0_27 = libgit2.overrideAttrs (oldAttrs: rec {
version = "0.27.8";
Copy link
Contributor

Choose a reason for hiding this comment

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

This version is insecure. You need to update to https://github.com/libgit2/libgit2/releases/tag/v0.27.10, or better https://github.com/libgit2/libgit2/releases/tag/v1.0.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or add

meta = oldAttrs.meta // {
  knownVulnerabilities = [
    "CVE-2019-1348"
    "CVE-2019-1349"
    "CVE-2019-1350"
    "CVE-2019-1351"
    "CVE-2019-1352"
    "CVE-2019-1353"
    "CVE-2019-1354"
    "CVE-2019-1387"
  ];
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched it to 0.27.10

Copy link
Contributor

Choose a reason for hiding this comment

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

Please also open an upstream issue since it is not clear how long it will remain supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created this issue : isacikgoz/gitin#63

@ofborg ofborg bot requested review from talyz, globin and fpletz April 29, 2020 19:32
@talyz
Copy link
Contributor

talyz commented May 3, 2020

@GrahamcOfBorg test gitlab

@kimat kimat mentioned this pull request May 21, 2020
Closed
@kimat kimat requested a review from doronbehar May 21, 2020 17:11
@doronbehar
Copy link
Contributor

All looks good but the last commit message has a typo. form -> from

doronbehar
doronbehar approved these changes May 22, 2020
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

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

Works as far as --help is concerned.

@kimat
Copy link
Contributor Author

kimat commented May 22, 2020

@doronbehar I just fixed the typo in the commit message of c270ecd.

@kimat kimat requested a review from jtojnar May 22, 2020 08:47
@kimat kimat changed the title gitin: init at 0.27.8 gitAndTools.gitin: init at 0.27.8 May 24, 2020
@talyz
Copy link
Contributor

talyz commented May 24, 2020

Works as expected in my tests. Thank you for your work!

@talyz talyz merged commit 985d726 into NixOS:master May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar approved these changes

@talyz talyz Awaiting requested review from talyz

@globin globin Awaiting requested review from globin

@fpletz fpletz Awaiting requested review from fpletz

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@kimat @talyz @doronbehar @jtojnar