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

go-moduls: clarify future of vend tool #102426

Closed
wants to merge 1 commit into from

Conversation

blaggacao
Copy link
Contributor

Motivation for this change

https://github.com/go-modules-by-example/index/blob/master/012_modvendor/README.md

  • Over time, the vend approach is likely to become deprecated.
  • Breadcrumb trail to lower R&D barrier for future pair of eyes (includig my own)

@@ -22,6 +22,7 @@
, deleteVendor ? false
# Whether to run the vend tool to regenerate the vendor directory.
# This is useful if any dependency contain C files.
# TODO: adapt to https://github.com/go-modules-by-example/index/blob/master/012_modvendor/README.md
Copy link
Contributor

@zowoq zowoq Nov 2, 2020

Choose a reason for hiding this comment

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

Wasn't this the previous approach that was replaced by the current implementation of buildGoModule?

Also, where did this document come from where was the TODO discussed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wasn't this the previous approach that was replaced by the current implementation of buildGoModule?

I don't know.

where was the TODO discussed?

In my head and in #102426 (comment) assuming that the upstream community is authoritative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

There was no previous approach.

I'm referring to #93513

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The motivation of this PR really has it all. TODO might or might not be the right token for it. but FUTURE is not in any common todo regex tooling, yet 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So all in all, I'm concluding as that it appears that leaving this code comment is a) valid & b) useful and c) likely going in the direction the community will be taking, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is also: nlewo@09ac010#r43788902

And #84826 (comment)

now.. 😉

Copy link
Contributor

@zowoq zowoq Nov 2, 2020

Choose a reason for hiding this comment

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

So all in all, I'm concluding as that it appears that leaving this code comment is a) valid & b) useful and c) likely going in the direction the community will be taking, no?

No, it is not.

As I've already said, we want to add goDeps / vgo2nix support to buildGoModule.

We don't want to re-add this method that (ab)uses FOD.

See discussion in #86376 which lead to #86465.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I unserstand where one branch of the discussion is heading to. Thank you!

Since it's clear folks will eventually change this, what alternative hint can we give, then at this line?

Please feel free to make a github suggestion to change this line to get this right.

Copy link
Contributor Author

@blaggacao blaggacao Nov 2, 2020

Choose a reason for hiding this comment

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

We don't want to re-add this method that (ab)uses FOD.

In that regard you are comparing apples to oranges, don't you? This method is no different from the current implementation, the only difference is it also fetches non go files that reside in a directory that does not have go files.

But as an aside, any fetch is a fixed output derivation if the source and transport cannot be trusted.

So why the heck is it bad to have two fixed output derivations if building a go module effectively involves two step fetching?

There is no chance one could catch the (real) problem of source mutability, anyways. For that nix community would have to host an instance of athens.

@SuperSandro2000
Copy link
Member

Closing based on discussion.

@blaggacao
Copy link
Contributor Author

blaggacao commented Jan 19, 2021

Yeah, too much tribal knowledge involved that makes it very ineffective to extrapolate any reasoning.

Thanks for closing!

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

Successfully merging this pull request may close these issues.

None yet

3 participants