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

Jump to bottom

x/build/cmd/updatestd: investigate a package-first update strategy, instead of module-first #41589

Open
dmitshur opened this issue Sep 23, 2020 · 3 comments
Open

x/build/cmd/updatestd: investigate a package-first update strategy, instead of module-first #41589

dmitshur opened this issue Sep 23, 2020 · 3 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Sep 23, 2020 •
edited

Patch set 1 of CL 256357 implemented a per-module update strategy for golang.org/x dependencies in the std and cmd standard library modules. At a high level:

  1. Determine build list (aka list of modules that are in module graph).

  2. Invoke a single go get command that selects desired new module versions (determined by target branch position on Gerrit) of golang.org/x modules:

    go get -d \
  golang.org/x/crypto@5c72a883971a4325f8c62bf07b6d38c20ea47a6a \
  golang.org/x/net@62affa334b73ec65ed44a326519ac12c421905e3 \
  golang.org/x/sys@af09f7315aff1cbc48fb21d21aa55d67b4f914c5 \
  golang.org/x/text@a8b4671254579a87fadf9f7fa577dc7368e9d009 \
  golang.org/x/tools@d647fc2532668b2b75a92f468487b8085e6ed58b

This issue is about investigating modifying the strategy to perform the update on a per-package level.

See the second part of @bcmills's comment on CL 256357 for more details, as well as my reply there.

What I'm imagining (but is incomplete and untested) is roughly:

  1. Determine list of packages that are not a part of the module (and in turn may have updates available).

    • This can either be done by reading vendor.txt file, or by running something like go list -f '{{if .Module}}{{.ImportPath}}{{end}}' all.

  2. For each package, determine which module provides it, figure out the desired version.

  3. Invoke a single go get command that requests packages at selected versions:

    go get -d \
  golang.org/x/sys/cpu@af09f7315aff1cbc48fb21d21aa55d67b4f914c5 \
  golang.org/x/text/secure/bidirule@a8b4671254579a87fadf9f7fa577dc7368e9d009 \
  golang.org/x/text/transform@a8b4671254579a87fadf9f7fa577dc7368e9d009 \
  golang.org/x/text/unicode/bidi@a8b4671254579a87fadf9f7fa577dc7368e9d009 \
  golang.org/x/text/unicode/norm@a8b4671254579a87fadf9f7fa577dc7368e9d009
  ...  # same for packages from all other modules

This is the tracking issue for investigating and implementing this as needed, and discussing the approach. It should be easier to do that here than in a code review.

I don't want to do this work now because there isn't a need yet (and I have higher priority work). But this will come up in the future. Filing an issue in the meantime.
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 23, 2020
@dmitshur dmitshur added this to the Backlog milestone Sep 23, 2020
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Sep 23, 2020
@dmitshur
Copy link
Contributor Author

I've spent some time thinking about this suggestion, and I understand it a lot better now. I also agree that a package-centric approach, if it is made robust, can be better and cleaner. However, I discovered a problem with it.

While trying to implement this new approach, I realized there is a potential pitfall: when using go list in package mode rather than module mode (no -m flag), package selection is done using the current build context. This can become a problem: imagine the operation is done in a GOOS=darwin environment, then some packages or their dependencies which are only a part of another environment will not be included in that context. Consider:

$ GOOS=darwin go list golang.org/x/sys/... | wc -l
3
$ GOOS=windows go list golang.org/x/sys/... | wc -l
10

(In theory, a package may have different dependencies depending on GOOS, GOARCH, CGO_ENABLED, and even compiler gc vs gccgo, since these all affect which .go files are selected.)

As far as I know, go list does not have a flag to list packages in a way that considers "any" build context. So go list couldn't be used reliably, and we'd need to read .go files manually, ignore ones with "// +build ignore" but otherwise consider all build constraints satisfied. This seems quite complicated. Listing modules avoids this problem for us, because modules already take into account all build contexts.

@bcmills Doe this make sense to you? Have you considered this factor? Do you have any suggestions for how we can make a package-centric approach work?

@bcmills
Copy link
Contributor

bcmills commented Oct 20, 2020

We have had other requests for a go list flag to ignore build constraints (from @dominikh and a few others, IIRC). I suspect that we will end up needing to support such a flag in cmd/go at some point anyway.

@dmitshur
Copy link
Contributor Author

dmitshur commented Oct 20, 2020 •
edited

Here is a strategy I am currently imagining if such a hypothetical flag is supported:

  1. Determine an import path pattern that matches packages contained in the current module. For the cmd module, it can be cmd/....
  2. Run go list -f '{{with .Module}}{{.Path}}{{end}}' -deps -test -ignore-build-constraints {pattern} to get a list of all module paths that are relevant (i.e., they contribute a direct or indirect dependent package to a package in the current module, considering all build contexts).
  3. Determine available updates for all modules. (If cmd/go: allow 'go list' to accept '-u' without '-m' #40676 is resolved, maybe this can be done as part of the previous step.)
  4. Invoke a single go get command that updates all relevant modules/packages (and none of the irrelevant ones) to the desired versions.

The outcome of this strategy seems good, but it is looks quite complicated to implement and maintain.

I'm not sure how lazy module loading (#36460) intersects with this. Maybe it will make it possible to implement this more simply after that's done?

@dmitshur dmitshur changed the title x/build/cmd/updatestd: investigate (at an appropriate time) a package-first update strategy, instead of module-first x/build/cmd/updatestd: investigate a package-first update strategy, instead of module-first Oct 20, 2020
@dmitshur dmitshur mentioned this issue Aug 25, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Builders x/build issues (builders, bots, dashboards) NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@dmitshur @bcmills @gopherbot and others