Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Autobuilder: always check the vendor directory works and if go.mod exists #312

Merged

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Aug 26, 2020

This is #310 with additions:

  • Always check vendor/ is usable if we're about to use it
  • Only use -mod when go.mod exists
  • Factor the paths using checkVendor (optional, could drop)

Locally verified this fixes a further 24 projects; proceeding to build a dist and check on all projects.

Sauyon Lee and others added 6 commits August 26, 2020 11:25
This only applies for module files for which no Go version has
been specified; Go will assume these should be parsed with the
latest Go version, which will cause them to fail if the vendor
directory has been generated with an old version of Go, as
the vendor/modules.txt will not meet the new requirements for
consistency.
This means dependency installation is still attempted when a vendor
directory is inconsistent.
These now follow the same route:

* Run a default or custom build script
* If needed, check if vendor/ is usable
* If it isn't, or if their build failed, install dependencies using go get etc

This commit shouldn't cause any behavioural change.
@smowton smowton requested a review from a team August 26, 2020 11:31
Also don't pass a blank argument to `go` when using an old version.
Copy link
Contributor

@max-schaefer max-schaefer left a comment

Choose a reason for hiding this comment

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

LGTM, though admittedly I'm deriving most of my confidence from Chris' tests, not my review. I'll leave it to Sauyon to merge this if he approves.

@@ -508,8 +508,13 @@ func main() {
}

var cmd *exec.Cmd
log.Printf("Running extractor command '%s %s ./...' from directory '%s'.\n", extractor, modMode, cwd)
cmd = exec.Command(extractor, modMode.String(), "./...")
if depMode == GoGetWithModules && modMode.String() != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if depMode == GoGetWithModules && modMode.String() != "" {
if depMode == GoGetWithModules && modMode != ModUnset {

That said, modMode can be unset by modMode = modModIfAvailable, so it's probably worth having a separate thing to track that.

Or, actually, we could move the logic for versions into ModMode.String() and then it would make everything else a bit nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned this up

The semver package requires versions of the form v1.2.3, and unhelpfully evaluates any malformed versions as equal.
case ModMod:
return "-mod=mod"
if semver.Compare(getEnvGoVersion(), "1.14") < 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You pass version above, but you don't seem to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry this is fixed in the commit after

@@ -58,6 +59,15 @@ func getEnvGoVersion() string {
return goVersion
}

// Returns the current Go version in semver format, e.g. v1.14.4
func getEnvGoSemVer() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any reason to keep getEnvGoVersion around, is there? I probably could have tried to test the version support...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for reporting the version at the top in the same terms as go version returns it, which I think is reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that makes sense.

@sauyon sauyon merged commit 1743dae into github:rc/1.25 Aug 27, 2020
@smowton
Copy link
Contributor Author

smowton commented Aug 27, 2020

Re-tested this afternoon with latest changes -- all looking good.

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

Successfully merging this pull request may close these issues.

3 participants