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

vend: Use nix-community fork of vend. #94574

Closed
wants to merge 1 commit into from
Closed

vend: Use nix-community fork of vend. #94574

wants to merge 1 commit into from

Conversation

c00w
Copy link
Contributor

@c00w c00w commented Aug 2, 2020 •
edited

Note that pkgs.govendor is currently taken by a package which is
orphaned by upstream. It's mildly confusing to have vend point to
nix-community/govendor, but I felt govendor was a better name for the
nix tool since it's easy to understand what's it's for, compared to vend
which won't really make sense to anyone who doesn't package go.

I manually checked that the same hashes were generated by the new tool.

Motivation for this change
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.

@c00w
vend: Use nix-community fork of vend.
34ae134 
Note that pkgs.govendor is currently taken by a package which is
orphaned by upstream. It's mildly confusing to have vend point to
nix-community/govendor, but I felt govendor was a better name for the
nix tool since it's easy to understand what's it's for, compared to vend
which won't really make sense to anyone who doesn't package go.
@c00w
Copy link
Contributor Author

c00w commented Aug 2, 2020

cc @Mic92 @zowoq

@zowoq
Copy link
Contributor

zowoq commented Aug 2, 2020

It's mildly confusing to have vend point to
nix-community/govendor

IMHO it is more than just mildly confusing.

I am not in favour for the name collision with another Go vendoring tool even if it is abandoned.

@ofborg ofborg bot requested a review from kalbasit August 2, 2020 22:19
@Mic92
Copy link
Member

Mic92 commented Aug 3, 2020 •
edited

It's mildly confusing to have vend point to
nix-community/govendor

IMHO it is more than just mildly confusing.

I am not in favour for the name collision with another Go vendoring tool even if it is abandoned.

Is there maybe a third name not used by anything? How about deepvendor as in deep copy? fullvendor? @zimbatm Help me out here :)

@zimbatm
Copy link
Member

zimbatm commented Aug 3, 2020

What's the issue with https://github.com/nomad-software/vend, are they not accepting patches?

Unless the project is a complete rewrite, I would keep the same name. It's not great, live with it :-D

If we have to fork it, I would also make sure that the repo is recorded as a fork of upstream for clarity's sake, and add an explanation to the README to explain why we need a long-term fork.

@zowoq
Copy link
Contributor

zowoq commented Aug 3, 2020

nomad-software/vend@dc54ecb Upstream seems to have implemented their own version of replacements.

@Mic92
Copy link
Member

Mic92 commented Aug 4, 2020 •
edited

What's the issue with nomad-software/vend, are they not accepting patches?

Unless the project is a complete rewrite, I would keep the same name. It's not great, live with it :-D

If we have to fork it, I would also make sure that the repo is recorded as a fork of upstream for clarity's sake, and add an explanation to the README to explain why we need a long-term fork.

The main motivation is to prevent checksum breaks. Upstream does not care about this too much and automatic updates by r-ryantm might introduce silent checksum changes. The tool itself is not overly complex that's why I think maintaining it our self is the better way to not break things in buildGoModule.

@zowoq
Copy link
Contributor

zowoq commented Aug 4, 2020

automatic updates by r-ryantm might introduce silent checksum changes.

There are a couple of different ways to prevent the bot sending PRs.

@zimbatm
Copy link
Member

zimbatm commented Aug 4, 2020 •
edited

In order of importance for me is:

  • keep the 1:1 mapping between the repo and package name. It's annoying to go resolve layers of indirections.
  • make the fork explicit

I would re-fork upstream into nix-community/vend, rename the repo to nix-community/nixpkgs-vend and update the README with a quick explanation about the fork. And then update nixpkgs and rename the package as well.

@zowoq
Copy link
Contributor

zowoq commented Aug 8, 2020 •
edited

I've checked a few packages and the upstream replacements seems to work.

Upstream has also removed fatih/color which was the only dependency. nomad-software/vend@bb65bee

Assuming that the upstream version is suitable for us to use I think we could just pin the version and disable the update bot.

Adding a warning should be sufficient to prevent accidental upgrades in the event someone does try to bump it manually.

Update:

Upstream vend works for the original six packages and only two of them need hash changes.

However it breaks telegraf.

@Mic92
Copy link
Member

Mic92 commented Aug 9, 2020

I've checked a few packages and the upstream replacements seems to work.

Upstream has also removed fatih/color which was the only dependency. nomad-software/vend@bb65bee

Assuming that the upstream version is suitable for us to use I think we could just pin the version and disable the update bot.

Adding a warning should be sufficient to prevent accidental upgrades in the event someone does try to bump it manually.

Update:

Upstream vend works for the original six packages and only two of them need hash changes.

However it breaks telegraf.

So the only patch left would be the mod-tidy one? @c00w what is your opinion about having the fork?

@zowoq
Copy link
Contributor

zowoq commented Aug 15, 2020

Once #95485 is merged telegraf won't require runVend.

I've opened to #95487 switch to upstream.

@zowoq zowoq mentioned this pull request Aug 15, 2020
Merged
10 tasks
@zowoq zowoq closed this in #95487 Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kalbasit kalbasit Awaiting requested review from kalbasit

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@c00w @zowoq @Mic92 @zimbatm