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

Test for vendor inconsistency and disable vendoring if it is found #308

Closed
wants to merge 82 commits into from

Conversation

sauyon
Copy link
Contributor

@sauyon sauyon commented Aug 25, 2020

This currently depends on #301 because I thought it would be easier to work on top of that than to merge later, though I can split it out if need be.

gagliardetto and others added 30 commits August 4, 2020 14:11
Add taint tracking for bufio and bytes packages
In a call of the form `f(xs...)`, when we say that `f` taints its 0th argument its ambiguous whether that means that it taints the slice `xs` or its 0th element `xs[0]`.

In practice, it's usually the latter, but we have no way of expressing that using our current `FunctionOutput` implementation.
Adds classes for some integer-parsing functions and a constant from
strconv, plus a class for calls to integer-parsing functions.
Teach extractor to distinguish calls with an ellipsis from calls without
Max Schaefer and others added 7 commits August 24, 2020 17:06
Unlike the old ODASA consistency queries, new consistency queries can have expected results, so there is no need to have special handling of files with expected errors.
@sauyon sauyon requested a review from a team August 25, 2020 08:00
@sauyon sauyon changed the base branch from main to rc/1.25 August 25, 2020 08:06
@sauyon
Copy link
Contributor Author

sauyon commented Aug 25, 2020

I've added a test for the Go version because Yorck mentioned offhand it might be nice to have support for older Go versions, so I thought I'd give myself a head-start on that in case we decide we do want to support versions < 1.14 in the future.

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

Looks good, will distcompare and test this with the list of failing projects now

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

One possible concern: we get Skipping dependency installation because a Go vendor directory was found. even if we're about to conclude the vendor directory is broken. We might want to make that check earlier so we can build just like we would in a non-vendored project including dependency installation.

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

Tested with 94 failing projects from lgtm.com: 79 successfully extracted with this patch. Details on the fails coming up.

Sauyon Lee added 3 commits August 25, 2020 03:35
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.
@sauyon
Copy link
Contributor Author

sauyon commented Aug 25, 2020

One possible concern: we get Skipping dependency installation because a Go vendor directory was found. even if we're about to conclude the vendor directory is broken. We might want to make that check earlier so we can build just like we would in a non-vendored project including dependency installation.

Right. I'd done this because I wanted the builds to happen first, but I hadn't thought about the dependency step. I've moved it to before that by doing it in both of the if branches for whether a build command was provided, but maybe this could do with some refactoring.

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

Failures are mostly projects with a vendor/ but not a vendor/modules.txt (they're using some other vendoring system, but they're surprised when Go 1.14+ tries to use their vendor directory anyway). I suggest we don't screw around with this PR further, but it would be a useful and easy followup to say that if vendor is present but modules.txt is not, and go version >= 1.14, then pass -mod=mod to avoid Go picking that vendor directory up anyhow (but since a different build system is clearly in use we accept our chances aren't great here)

@sauyon
Copy link
Contributor Author

sauyon commented Aug 25, 2020

Hmm, I assume they also have go.mod files? I did read at some point that people were using vendor directories instead of replace directives pointing to local repos for some reason, so it might be something along those lines.

@sauyon
Copy link
Contributor Author

sauyon commented Aug 25, 2020

I suggest we don't screw around with this PR further...

Er, I'll roll that back and put it up in a separate PR.

Also, we might want to add a bunch of these edge cases to our extractor tests since this seems like something that's likely to break as the Go team decides what they do and don't like in the future.

EDIT: I'll probably do this tomorrow since it's rather... early (?) now.

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

Sounds good on all fronts :) I'll roll an lgtm.com test-all-project for this PR, probably complete end of tomorrow or so.

@smowton
Copy link
Contributor

smowton commented Aug 25, 2020

Note this is currently based on main but targeting rc/1.25; I'll fix that if and when this passes muster on lgtm.com

@smowton
Copy link
Contributor

smowton commented Aug 26, 2020

Distcompare (likely applies to #310 and #312 as well): this adds between 3% and 14% to the build time for "big" projects, e.g. aws-sdk-go is the worst. Considering that repo does not have a vendor directory, this seems unlikely to be real. No result changes, as expected.

@max-schaefer
Copy link
Contributor

I take it this is superseded by #312? If so, let's close this to simplify things.

@sauyon sauyon closed this Aug 27, 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.

7 participants