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

Check dependencies before skipping dependency installation#369

Merged
smowton merged 2 commits intogithub:mainfrom
sauyon:checkdeps
Nov 11, 2020
Merged

Check dependencies before skipping dependency installation#369
smowton merged 2 commits intogithub:mainfrom
sauyon:checkdeps

Conversation

@sauyon
Copy link
Contributor

@sauyon sauyon commented Oct 8, 2020

Ran into some issues when testing a few projects where builds were unrelated to Go / exited 0 despite failing, so I wrote this quick change.

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

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

Can you add a test to extractor-tests exercising this case?

if !buildSucceeded || util.DepErrors("./...", modMode.argsForGoVersion(getEnvGoSemVer())...) {
// Build failed or there are still dependency errors; we'll try to install dependencies
// ourselves
log.Println("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Printing a blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd failed to push, whoops.

modMode = ModVendor
} else if util.DirExists("vendor") {
modMode = ModMod
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The check at line 286 is now unnecessary, right? Move all of the following if-block inside this if depMode == GoGetWithModules {

@sauyon
Copy link
Contributor Author

sauyon commented Oct 9, 2020

Can you add a test to extractor-tests exercising this case?

We can't test the autobuilder with codeql test at the moment. I think it would be nice to have an action to do that; how tied into the internal repo are your autobuilder tests?

@smowton
Copy link
Contributor

smowton commented Oct 9, 2020

Doh right, yes, makes sense. In that case how about making a branch derived from this one that fails the build with a distinctive error when (a) make or similar appears to succeed, but (b) go list reports continued dependency trouble? Then if you run an LGTM distribution test with that build you should get a list of projects that exercise this path and which can be dist-compare'd to check what this does to their results?

I think this is likely to improve things, just wary of introducing duplicate copies of libraries because make has vendored them somewhere then go get fetches another copy... that sort of thing.

@sauyon
Copy link
Contributor Author

sauyon commented Oct 29, 2020

This is actually the case for almost half the projects on staging, so it seems like a sensible thing to do.

Sauyon Lee added 2 commits November 9, 2020 02:13
Sometimes build scripts succeed without installing dependencies, for
example if they are unrelated to Go or if they simply always exit
successfully. Therefore, added a check that dependencies at least
resolve before skipping dependency installation.
@sauyon
Copy link
Contributor Author

sauyon commented Nov 11, 2020

Distribution build looks all good, with one project failure due to unrelated reasons. (The repository no longer exists)

@smowton smowton merged commit 82a5b5f into github:main Nov 11, 2020
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.

2 participants