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

Hook go-licenses into tidy again #21353

Merged
merged 11 commits into from Oct 10, 2022
Merged

Hook go-licenses into tidy again #21353

merged 11 commits into from Oct 10, 2022

Conversation

silverwind
Copy link
Member

Running it as part of the build is really unnecessary because we have a valid output file in the repo and assuming go dependencies do not change unless go.mod also changes, tidy really is the best target to run the license generation after.

Also, regenerate the file as I missed to do so during the chroma update, and mark all json files in assets as generated.

Running it as part of the build is really unnecessary because we have a
valid output file in the repo and assuming go dependencies do not change
unless go.mod also changes, tidy really is the best target to run the
license generation after.

Also, regenerate the file as I missed to do so during the chroma update,
and mark all json files in assets as generated.
@silverwind silverwind added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Oct 6, 2022
@silverwind
Copy link
Member Author

Test failure is unrelated, see #21355.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 6, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 6, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 6, 2022
@zeripath
Copy link
Contributor

zeripath commented Oct 6, 2022

running make generate on the current head already produces a different assets/go-license.json

I run make vendor on checkout so that I can solve bugs.

Adding this into tidy will result in making checkout even slower.

@silverwind
Copy link
Member Author

silverwind commented Oct 6, 2022

make vendor is, since recently decoupled from make tidy, so this will not run at all. And even if you run make tidy directly, it will only run the ~15sec licence generation if go.mod changed during the checkout (and with it, licenses could very likely change).

CI will always run tidy-check, and with it we can ensure the licenses stay up-to-date.

@silverwind
Copy link
Member Author

silverwind commented Oct 6, 2022

Actually, I think I need to force CI to run the target because depending on git checkout order, the target might be skipped otherwise as seen in the recent run:

make[1]: 'assets/go-licenses.json' is up to date.

@silverwind
Copy link
Member Author

Added a touch, which will be enough so make sees the target as out of date and always runs it.

@silverwind
Copy link
Member Author

silverwind commented Oct 6, 2022

I just learned that make has the --always-make flag to achieve the same, so use that instead of touch hacks.

  -B, --always-make           Unconditionally make all targets.

@6543 6543 added this to the 1.18.0 milestone Oct 7, 2022
@zeripath
Copy link
Contributor

zeripath commented Oct 8, 2022

Wouldn't this mean you could make go-licenses part of tidy-check?

diff --git a/Makefile b/Makefile
index 3a9025746..f56997e82 100644
--- a/Makefile
+++ b/Makefile
@@ -403,12 +403,9 @@ unit-test-coverage:
 	@$(GO) test $(GOTESTFLAGS) -timeout=20m -tags='$(TEST_TAGS)' -cover -coverprofile coverage.out $(GO_PACKAGES) && echo "\n==>\033[32m Ok\033[m\n" || exit 1
 
 .PHONY: tidy
-tidy:
-	$(eval MIN_GO_VERSION := $(shell grep -Eo '^go\s+[0-9]+\.[0-9.]+' go.mod | cut -d' ' -f2))
-	$(GO) mod tidy -compat=$(MIN_GO_VERSION)
-	@$(MAKE) --no-print-directory $(GO_LICENSE_FILE)
+tidy: go.mod go.sum $(GO_LICENSE_FILE)
 
-vendor: go.mod go.sum
+vendor: tidy
 	$(GO) mod vendor
 	@touch vendor
 
@@ -424,6 +421,12 @@ tidy-check: tidy
 .PHONY: go-licenses
 go-licenses: $(GO_LICENSE_FILE)
 
+go.sum: go.mod
+	$(eval MIN_GO_VERSION := $(shell grep -Eo '^go\s+[0-9]+\.[0-9.]+' go.mod | cut -d' ' -f2))
+	$(GO) mod tidy -compat=$(MIN_GO_VERSION)
+	@touch go.sum
+
+
 $(GO_LICENSE_FILE): go.mod go.sum
 	-$(GO) run $(GO_LICENSES_PACKAGE) save . --force --save_path=$(GO_LICENSE_TMP_DIR) 2>/dev/null
 	$(GO) run build/generate-go-licenses.go $(GO_LICENSE_TMP_DIR) $(GO_LICENSE_FILE)

@silverwind
Copy link
Member Author

tidy can and often will alter go.mod but that'd not be correctly pictured in these dependencies. Probably won't matter in practice.

I'm still not sure why you want to re-introduce the tidy dependency to vendor again. In my eyes, tidy and vendor should be independant and if you really want to run them back-to-back, you should use make tidy vendor.

@6543 6543 merged commit 94037ad into go-gitea:main Oct 10, 2022
@silverwind silverwind deleted the tidylic branch October 10, 2022 18:49
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 11, 2022
* upstream/main:
  Add user/organization code search (go-gitea#19977)
  Stop logging CheckPath returns error: context canceled (go-gitea#21064)
  Hook go-licenses into tidy again (go-gitea#21353)
  Fix missing left and right carets in TRANSLATORS (go-gitea#21397)
  Fix calls to i18n in templates (go-gitea#21394)
  Update JS dependencies and eslint config (go-gitea#21388)
  Allow creation of OAuth2 applications for orgs (go-gitea#18084)
  Fix typos in PullRequestMergeForm.vue header comment (go-gitea#21378)
  Use weighted algorithm for string matching when finding files in repo (go-gitea#21370)
  Bump playwright to 1.26.1 (go-gitea#21357)
  npm package registry support for `bin` (go-gitea#21372)
  Removed one extra whitespace in footer after "Template" (go-gitea#21364)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants