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-upfetch: init at 0.1.0 #56311

Closed
wants to merge 1 commit into from
Closed

nix-upfetch: init at 0.1.0 #56311

wants to merge 1 commit into from

Conversation

@msteen
Copy link
Contributor

@msteen msteen commented Feb 24, 2019

Motivation for this change

Update a call made to a fetcher function call and its surrounding bindings.

Based on nix-prefetch (to locate the fetcher call and update the fetcher arguments passed to it) and rnix (to parse the file containing the fetcher call and update fetcher arguments and surrounding bindings).

Makes it really convenient to update Nix packages as described in the README:
https://github.com/msteen/nix-upfetch

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

};

in stdenv.mkDerivation rec {
name = "${pname}-${version}";

This comment has been minimized.

@jtojnar

jtojnar Feb 24, 2019
Contributor

This is not necessary – mkDerivation does that by default.

This comment has been minimized.

@msteen

msteen Feb 24, 2019
Author Contributor

I had it for backwards compatibility, since I developed / tested it under 18.09, but since this PR is based on master, I can indeed remove it.

This comment has been minimized.

@msteen

msteen Feb 24, 2019
Author Contributor

I also tried removing it at rustPlatform.buildRustPackage, but it still expects a name attribute, so I will keep it there.

];

configurePhase = ''
. configure.sh

This comment has been minimized.

@jtojnar

jtojnar Feb 24, 2019
Contributor

Does [configureScript] (

$configureScript "${flagsArray[@]}"
) not work?

This comment has been minimized.

@msteen

msteen Feb 24, 2019
Author Contributor

It probably will, simply did not know about it, thanks for the suggestion!

This comment has been minimized.

@msteen

msteen Feb 24, 2019
Author Contributor

Tried it, but since it is simply setting a variable, turning it into a configure scripts complicates matters unnecessary, so I am keeping it as it is for now, unless I find some better way.

'';

installPhase = ''
lib=$out/lib/${pname}

This comment has been minimized.

@jtojnar

jtojnar Feb 24, 2019
Contributor

Why not create a makefile in the project?

This comment has been minimized.

@msteen

msteen Feb 24, 2019
Author Contributor

I have limited Makefile experience, what would this gain me? If it's worth doing, could you maybe give an example of what you have in mind?

This comment has been minimized.

@aanderse

aanderse Jul 13, 2019
Contributor

I think one reason might be that most of that code in installPhase could remain upstream. I get that it seems silly to you as you are upstream... but keeping code like that upstream wherever possible is the NixOS way.

We're so close on this PR. It would be nice to see it to completion.

This comment has been minimized.

@msteen

msteen Jul 13, 2019
Author Contributor

@aanderse If you want me to add a Makefile, I will, but I will not be attempting this myself without being pointed to some Makefile that does something similar to what I need to do, because I have only limited experience with making Makefiles (small C projects).

This comment has been minimized.

@aanderse

aanderse Jul 21, 2019
Contributor

@msteen Honestly I was just trying to triage old/stale PRs... @jtojnar Anything to add?

This comment has been minimized.

@ryantm

ryantm Jan 4, 2020
Member

If @msteen wants to make nix-upfetch available in different package repositories in the future, he can upstream his script and update this derivation.

@msteen msteen force-pushed the msteen:nix-upfetch branch from 7731630 to ddfa92e Feb 24, 2019
@msteen msteen force-pushed the msteen:nix-upfetch branch from ddfa92e to b707312 Mar 2, 2019
'';

installPhase = ''
lib=$out/lib/${pname}

This comment has been minimized.

@ryantm

ryantm Jan 4, 2020
Member

If @msteen wants to make nix-upfetch available in different package repositories in the future, he can upstream his script and update this derivation.


let
nix-update-fetch = rustPlatform.buildRustPackage rec {
name = "${pname}-${version}";

This comment has been minimized.

@ryantm

ryantm Jan 4, 2020
Member

Suggested change
name = "${pname}-${version}";

src = fetchFromGitHub {
owner = "msteen";
repo = "nix-update-fetch";

This comment has been minimized.

@ryantm

ryantm Jan 4, 2020
Member

Suggested change
repo = "nix-update-fetch";
repo = pname;
@msteen
Copy link
Contributor Author

@msteen msteen commented Jan 5, 2020

I never had any plans of packaging it for other distros. However since the suggestion to remove the few lines of script that might be reusable in other distros has been turned into a blocking issue, I choose to close this PR, because my local version has long since diverged from this version. The local version fits my needs, but in its current state is not ready to be published and at the moment I have no interest in putting in the time to patch up this old version or make my local version ready to be published again. So unless someone wants to see it in nixpkgs other than me, I will just make a new PR once I find the time and interest to make changes to the project again.

@msteen msteen closed this Jan 5, 2020
@ryantm
Copy link
Member

@ryantm ryantm commented Jan 5, 2020

@msteen sorry if my short comment gave the wrong impression, but I do not think the script upstreaming thing is a blocking issue and didn't mean to give that impression. That said, based on your last post, it sounds fine to close this.

@msteen
Copy link
Contributor Author

@msteen msteen commented Jan 5, 2020

@ryantm Well, regardless of original intent, whether it meant it to be purely a suggestion or a blocking issue, it was as far as I know the only reason it was not merged. And since everybody else kept on focusing on this suggestion as if it had to be cleared to be merged, it actually did turn into a block issue. Regardless of this, I indeed explained why by now it makes sense to just close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.