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

nix-update-source: init at 0.3.0 #22660

Merged
merged 1 commit into from
Apr 30, 2017
Merged

Conversation

timbertson
Copy link
Contributor

@edolstra this one will need a review from you since it builds on #21734 (which was reverted)

Reintroduce nix-update-source at version 0.3.0. This version includes support for modifying a nix file inline[*] instead of using a separate JSON file. This PR also includes a passthru.updateScript for nix-update-source itself, as an example of JSON-less usage.

The version retains support for importing JSON files, since that's still useful for non-nixpkgs sources which prefer that method. But there's a comment warning people not to use it nixpkgs proper. I'm hoping this is an acceptable compromise.

[*]: it has little actual knowledge of nix syntax, because I don't have a real nix parser available - it relies on the nix file being formatted like a typical derivation

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

'';
};
meta = {
description = "Utility to autimate updating of nix derivation sources";
Copy link
Contributor

Choose a reason for hiding this comment

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

typo "autimate"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@joachifm
Copy link
Contributor

I wonder if this has been made obsolete by b53c53b cc @dezgeg this is the same idea, right?

@timbertson
Copy link
Contributor Author

@joachifm they're similar, but I wouldn't say that either makes the other obsolete. That script does a very particular modification, in a somewhat hacky way. This is a utility which can be used outside nix, allows reading/writing in a machine readable (json) format and should generally be extensible with update strategies beyond what can be implemented with sed. There's some stuff you could achieve with either tool, but there's also plenty you couldn't.

@edolstra , ping? I believe this version addresses your concerns with the previous PR, so if you're happy with this version it'd be great to get this merged for good.

@joachifm
Copy link
Contributor

I think it'd be good if we could standardise on this tool, then, if it is superior; having two tools that are almost-but-not-quite-the-same-thing seems unfortunate.

@pSub pSub added the 8.has: package (new) This PR adds a new package label Feb 20, 2017
@timbertson
Copy link
Contributor Author

Ping, @edolstra ? Sorry to keep troubling you, but this probably needs your OK given what happened last time...

@timbertson
Copy link
Contributor Author

@edolstra, can you please comment? I'm trying not to pester you too frequently but you're the only person who can unblock this, and it's been over a month with no word. I could try getting other admins to +1 the PR, but without your OK it runs the risk of getting reverted again.

To answer your question @joachifm, no I don't believe it's necessary to standardise on one such tool right now. The original PR discussed at length people's different wishes / goals for this kind of tool, and trying to please everyone at once before anyone can use a tool seems like a sure way to get nothing done. Just as there are dozens of packages in nix which accomplish similar things, it doesn't seem reasonable that nix require only one tool which is capable of updating package definitions.

@timbertson
Copy link
Contributor Author

Pinging a few project members who were active on the original PR: can anyone help me get this PR moving? I believe I've addressed the issue that caused @edolstra to revert the original, but I haven't been able to get a response for 6+ weeks :(

/cc @FRidh @copumpkin @domenkozar @globin @7c6f434c

};
meta = {
description = "Utility to automate updating of nix derivation sources";
maintainers = with lib.maintainers; [ timbertson ];
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a license?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (MIT)

Copy link
Member

@7c6f434c 7c6f434c left a comment

Choose a reason for hiding this comment

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

I guess I am in favour of that, and I think it generally fixes the exact points @edolstra raised about the previous version, not completely sure if there are more complicated issues that will get raised with this one…

@7c6f434c 7c6f434c merged commit ebc9e7a into NixOS:master Apr 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants