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

Fix writing vendor/modules.txt to deal with subpackages #29

Merged
merged 4 commits into from
Mar 21, 2025

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Mar 19, 2025

The file format isn't clearly documented, but this example from the go project shows that what we were producing before wasn't correct.

Note that a lot of this code was written by copilot, so should perhaps merit greater scrutiny.

@owen-mc owen-mc requested review from a team and Copilot March 19, 2025 16:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the generation of vendor/modules.txt by updating the code to deal with subpackages while replacing deprecated ioutil functions with equivalent os functions.

  • Replaces ioutil functions with os functions across multiple files.
  • Introduces helper functions to scan Go source files and identify used submodules.
  • Updates import statements to remove unused packages and add necessary ones (e.g. go/parser, regexp).

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
vendor.go Implements submodule extraction and writes explicit module/submodule entries.
reflect.go Replaces temporary file and directory creation methods using ioutil with os.
util.go Uses os.ReadDir instead of ioutil.ReadDir for improved file reading handling.
depstubber.go Updates file reading to use os.ReadFile, removing the dependency on ioutil.

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@owen-mc
Copy link
Contributor Author

owen-mc commented Mar 20, 2025

I don't think depstubber has any tests. I've tried this on one folder in github/codeql and it fixed the problem I was having. Before merging I should try it in more (all, if possible) to check that it doesn't cause any regressions.

Copy link

@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.

Looks plausible

@owen-mc
Copy link
Contributor Author

owen-mc commented Mar 20, 2025

I reran all the depstubber commands stored as go-generate comments in codeql/go. This PR contains the results. After reverting changes which overrode manual edits all the tests passed.

@owen-mc owen-mc force-pushed the owen-mc/subpackes-in-modules-txt branch from d8275a3 to 3ac77e0 Compare March 20, 2025 12:46
@owen-mc
Copy link
Contributor Author

owen-mc commented Mar 20, 2025

One case where this new logic would fall over: nested go modules. If you have a go.mod in a child dir of a go.mod then it makes a new module. For a project which imports packages from both modules, we would then attribute a package of the inner module to both modules. However, I'm not really sure why you'd have nested go modules., and I have never seen this done in a library. And it should be relatively easy to fix manually. I propose that we don't try to fix depstubber for this case until we actually see it happen at least once in the wild.

@owen-mc
Copy link
Contributor Author

owen-mc commented Mar 21, 2025

I added a commit to sort the packages. This matches what go mod vendor does, making it easier to compare. And it also just makes it easier to read. The effect on our stubs for codeql/go can be seen in this commit.

@owen-mc owen-mc merged commit 0fd7e1e into main Mar 21, 2025
4 checks passed
@owen-mc owen-mc deleted the owen-mc/subpackes-in-modules-txt branch March 21, 2025 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants