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

buildGoModule: allow modSha256 #93624

Closed
wants to merge 8 commits into from
Closed

buildGoModule: allow modSha256 #93624

wants to merge 8 commits into from

Conversation

zowoq
Copy link
Contributor

@zowoq zowoq commented Jul 22, 2020

I'm not really happy about proposing this but I think we need some way of reliably dealing with these cases when go mod vendor doesn't work that doesn't require manual intervention.

Allowing the use of the previous implementation as a last resort seems the easiest approach.

cc @kalbasit @marsam @Mic92

@zowoq zowoq requested a review from mmahut as a code owner July 22, 2020 06:20
@kalbasit
Copy link
Member

I don't have strong feelings either way tbh. This is an acceptable change as far as I am concerned.

@Mic92
Copy link
Member

Mic92 commented Jul 22, 2020

cc @c00w

@Mic92
Copy link
Member

Mic92 commented Jul 22, 2020

So this is basically the opposite of #93513

@zowoq
Copy link
Contributor Author

zowoq commented Jul 22, 2020

cc ...

We should see about getting a @NixOS/golang

So this is basically the opposite of #93513

Yeah.

I want to get rid of modSha256 but until runVend or some other alternative works properly we may as well just use what we know works.

@zowoq
Copy link
Contributor Author

zowoq commented Jul 22, 2020

With 20.09 getting closing this will at least buy us another few months to find a reliable solution.

Copy link
Contributor

@c00w c00w left a comment

Choose a reason for hiding this comment

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

I think this is completely ridiculous.

@zowoq is the entire reason we haven't been able to merge runVend, so him claiming we're in some sort of helpless situation here seems extremely duplicitious.

Additionally, it's generally doable to fix specific issues with the vendor folder (as this code shows), in a fairly straightforward way, so I don't understand what benefit we get by undoing the go vendor work.

@Mic92
Copy link
Member

Mic92 commented Jul 22, 2020

@c00w I am not sure they are aware of you progress. I suggest @zowoq to have another look at your PR.

@Mic92
Copy link
Member

Mic92 commented Jul 22, 2020

I would be actually in favor of having vendoring to work with @c00w solution rather than having to maintain to different module implementation.

@zowoq
Copy link
Contributor Author

zowoq commented Jul 27, 2020

Updated this to use https://github.com/goware/modvendor.

The vendor command is a bit long but it seems to work.

@@ -21,6 +21,8 @@
# Whether to delete the vendor folder supplied with the source.
, deleteVendor ? false

, modvendorCopy ? false
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 mention this in doc/languages-frameworks/go.xml?

@Mic92
Copy link
Member

Mic92 commented Jul 27, 2020

What is the difference between this and vend?

@zowoq
Copy link
Contributor Author

zowoq commented Jul 27, 2020

Can you mention this in doc/languages-frameworks/go.xml?

I'll add something to the docs if we decide to go with this PR.

What is the difference between this and vend?

It's a simpler tool. It doesn't wrap go commands, it just copies files.

Downside is that it is a simple tool and we need to tell it to what to copy. Also, if we ever need to add more file types for whatever reason we may end up having to fix hashes because if those new file types exist in current packages they weren't being copied.

I'm not sure if this is better approach than vend or not, if we can get the replace problems fixed it may be better to stick with vend.

@Mic92
Copy link
Member

Mic92 commented Jul 27, 2020

Can you mention this in doc/languages-frameworks/go.xml?

I'll add something to the docs if we decide to go with this PR.

What is the difference between this and vend?

It's a simpler tool. It doesn't wrap go commands, it just copies files.

Downside is that it is a simple tool and we need to tell it to what to copy. Also, if we ever need to add more file types for whatever reason we may end up having to fix hashes because if those new file types exist in current packages they weren't being copied.

I'm not sure if this is better approach than vend or not, if we can get the replace problems fixed it may be better to stick with vend.

Having an explicit list does not sounds like a good idea. However I am concerned that if we ever add any file to the whitelist, we might break hashes. Would it be a bad idea to just add everything?

@zowoq
Copy link
Contributor Author

zowoq commented Jul 27, 2020

Would it be a bad idea to just add everything?

We can add as many file types as we want but anything else would require patching.

@zowoq zowoq closed this Jul 29, 2020
@zowoq zowoq deleted the gomod branch August 2, 2020 09:55
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.

4 participants