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

build: remove Go workspace files #407

Merged
merged 2 commits into from
Apr 25, 2023
Merged

build: remove Go workspace files #407

merged 2 commits into from
Apr 25, 2023

Conversation

antoineco
Copy link
Contributor

@antoineco antoineco commented Apr 25, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran make fmt on your commit series before opening this PR;
  • Updated relevant documentation.

Description of changes

The go.work file is currently misused; it references standalone tool modules which aren't imported by the kraftkit module.

The 'replace' directive inside kraftkit's go.mod file is also superfluous; the entry inside go.work is sufficient for kraftkit to locate the vendored revision.

Ref. https://go.dev/ref/mod#workspaces

+9
-1,003 

🙂


Update

Go workspace files removed entirely. See commit message(s) for the rationale.

@antoineco antoineco requested a review from nderjung April 25, 2023 07:02
The go.work file is currently misused[1]; it references standalone tool
modules which aren't imported by the kraftkit module.

The 'replace' directive inside kraftkit's go.mod file is also
superfluous; the entry inside go.work is sufficient for kraftkit to
locate the vendored revision.

[1]: https://go.dev/ref/mod#workspaces

Signed-off-by: Antoine Cotten <[email protected]>
@craciunoiuc
Copy link
Member

Would this also fix go.work.sum always updating itself? 😅

@antoineco
Copy link
Contributor Author

antoineco commented Apr 25, 2023

@craciunoiuc it might, since the only local module required by kraftkit is git2go, which version is not changing due to manual vendoring.

In fact, we could use a simple require inside go.mod pointing at the vendored module instead of using a go.work file, and things would keep working just fine.

go.work is a nice way to iterate on local dependencies without having to push these to Git (like a project-global replace), but I've seen several people recommend to exclude it from version control, among them GitHub themselves (github/gitignore#3884).

@craciunoiuc
Copy link
Member

Okay makes sense

Also looked over how go.work should be used and this PR seems a good idea 👍

Workspaces are mainly meant for working on local development versions of
modules. Putting these files under version control is against that
purpose.

For locally vendored modules such as libgit2/git2go, a 'replace'
directive inside go.mod is more appropriate.

Signed-off-by: Antoine Cotten <[email protected]>
@antoineco
Copy link
Contributor Author

Update: I removed the Go workspace files in a second commit. After reading extensively about this feature, it is clear to me that putting go.work under version control is both a misuse and an overhead.

@antoineco antoineco changed the title build: remove tools from Go workspace build: remove Go workspace files Apr 25, 2023
@nderjung nderjung added kind/maintenance Issue or PR is general repository maintenance kind/fix This PR fixes an issue or bug labels Apr 25, 2023
Copy link
Member

@nderjung nderjung left a comment

Choose a reason for hiding this comment

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

go.work (and its byproduct go.work.sum) are artifacts from creating the vendor/ directory which was previously renamed to third_party/. This change removes a now unnecessary file (and good riddance!) Thanks a lot!

Approved-by: Alexander Jung [email protected]

@nderjung nderjung merged commit 8fb2ec3 into unikraft:staging Apr 25, 2023
@antoineco antoineco deleted the go-work-cleanup branch April 25, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/fix This PR fixes an issue or bug kind/maintenance Issue or PR is general repository maintenance
Projects
Status: 🚀 Done
Development

Successfully merging this pull request may close these issues.

3 participants