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.

Sign up for GitHub

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

Move go-licenses to generate and separate generate into a frontend and backend component #21061

Merged
merged 11 commits into from
Sep 5, 2022
Merged

Move go-licenses to generate and separate generate into a frontend and backend component #21061

merged 11 commits into from
Sep 5, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Sep 4, 2022 •
edited
Loading

The go-licenses make task introduced in #21034 is being run on make vendor
and occasionally causes an empty go-licenses file if the vendors need to
change. This should be moved to the generate task as it is a generated file.

Now because of this change we also need to split generation into two separate
steps:

  1. generate-backend
  2. generate-frontend

In the future it would probably be useful to make generate-swagger part of generate-frontend but it's not tolerated with our .drone.yml

Ref #21034

Signed-off-by: Andrew Thornton art27@cantab.net

@zeripath
Move go-licenses to generate
2c8a1a2 
The go-licenses check introduced in go-gitea#21034 is being run on make vendor
and occassionally causes an empty go-licenses file if the vendors need to
change. This should be moved to the generate task as it is a generated file.

Ref go-gitea#21034

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Sep 4, 2022
@zeripath zeripath added this to the 1.18.0 milestone Sep 4, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 4, 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 Sep 4, 2022
@zeripath
Copy link
Contributor Author

zeripath commented Sep 4, 2022

ah this will require a little more finesse...

@zeripath
Only update the generated files if necessary
e7396ae 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Separate out generate into a frontend and backend
2aab729 
Make generate-swagger part of generate-frontend too

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
commit the results of running fmt
055e3d2 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath changed the title Move go-licenses to generate Move go-licenses to generate and separate generate into a frontend and backend component Sep 4, 2022
@zeripath
OK we cannot have the swagger be in the frontend generation so move i…
fe0218e 
…t to backend

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath force-pushed the move-gen-licenses-to-generate branch from b17f481 to fe0218e Compare September 4, 2022 19:40
@zeripath
do not rely on bindata for generate-swagger
2905adf 
Signed-off-by: Andrew Thornton <art27@cantab.net>
delvh
delvh reviewed Sep 4, 2022
View reviewed changes
modules/charset/invisible/generate.go Outdated Show resolved Hide resolved
modules/charset/ambiguous/generate.go Outdated Show resolved Hide resolved
Makefile
Comment on lines +286 to +288
generate-swagger: $(SWAGGER_SPEC)

$(SWAGGER_SPEC): $(GO_SOURCES_NO_BINDATA)
Copy link
Member

Choose a reason for hiding this comment

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

Why not directly

Suggested change
generate-swagger: $(SWAGGER_SPEC)
$(SWAGGER_SPEC): $(GO_SOURCES_NO_BINDATA)
generate-swagger: $(GO_SOURCES_NO_BINDATA)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope.

That won't do the right thing.

Try it

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I don't have to understand why, but I accept it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to do with the dependencies - the SWAGGER_SPEC line is creating a file that acts as evidence for the build job. If all sources are older than the SWAGGER_SPEC file it doesn't need to be generated

zeripath and others added 2 commits September 4, 2022 23:09
@zeripath @delvh
Update modules/charset/invisible/generate.go
b824367 
Co-authored-by: delvh <dev.lh@web.de>
@zeripath @delvh
Update modules/charset/ambiguous/generate.go
be6bc12 
Co-authored-by: delvh <dev.lh@web.de>
zeripath
zeripath commented Sep 5, 2022
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Sep 5, 2022

make L-G-T-M work

@lunny lunny merged commit 8080e23 into go-gitea:main Sep 5, 2022
@zeripath zeripath deleted the move-gen-licenses-to-generate branch September 5, 2022 08:00
@zeripath
Copy link
Contributor Author

zeripath commented Sep 5, 2022

There's still an intermittent issue with the go-licenses make target that appears to drop the code.gitea.io license from time to time.

@silverwind do you have any ideas what's happening here?

@silverwind
Copy link
Member

Not sure I understand why these changes here were all necessary, but I'd probably just have dropped the tidy dependency of vendor. Reason being that tidy is unnecessary in the release pipeline as the complicance pipeline already ensures that tidy is consistent.

@silverwind
Copy link
Member

silverwind commented Sep 5, 2022 •
edited
Loading

@silverwind do you have any ideas what's happening here?

Hmm, I could only imagine that the go-licenses tools fails to download sometimes, but I think it's using GOPROXY so it should work even if code.gitea.io is down. You can remove the 2>/dev/null in the Makefile which will make that tool spew its errors (there are many unrelated ones)

@silverwind
Copy link
Member

If automating this proves too difficult, we could also opt to make the target standalone without any dependants and just have it as a manual task after dependency update.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 6, 2022
@zjjhot
Merge remote-tracking branch 'upstream/main'
b38d134 
* upstream/main:
  Fix sub folder in repository missing add file dropdown (go-gitea#21069)
  [skip ci] Updated translations via Crowdin
  Add missing volume to test-e2e (go-gitea#21079)
  Fix delete user missed some comments (go-gitea#21067)
  Remove insecure flag from curl (go-gitea#21074)
  Update curl usage in API docs (go-gitea#21071)
  Move go-licenses to generate and separate generate into a frontend and backend component (go-gitea#21061)
@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.
Reviewers

@delvh delvh delvh approved these changes

@jolheiser jolheiser Awaiting requested review from jolheiser

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@zeripath @lunny @silverwind @jolheiser @delvh @GiteaBot