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

Frontend build improvements, hopefully fix Fomantic build #10576

Merged
merged 1 commit into from Mar 3, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 2, 2020

  • add package-lock.json to webpack/fomantic prereqs making them always rebuild when dependencies change.
  • remove FOMANTIC_EVIDENCE. It seemed better to just track a few output files instead.
  • delete fomantic output files before build to prevent possible bugs in fomantic's build.
  • resolve WEBPACK_SOURCES only once for performance
  • reorder variables for clarity
  • use clean-all in Dockerfile
  • detect busybox for find syntax

Fixes: #10569
Fixes: #10565
Fixes: #10570
Fixes: #10568

@silverwind
Copy link
Member Author

Prereq tracking still seems fine after this, e.g. I can run make fomantic && make fomantic or make webpack && make webpack and the second one skips on my machine in all cases.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2020
@silverwind
Copy link
Member Author

silverwind commented Mar 2, 2020

Added $(NODE_MODULES_EVIDENCE) in place of node_modules. I think it's unfortunately inevitable to use such a file because when tools write into node_modules, its timestamp may not accurately reflect when it was last updated anymore, which in turn could skip a update of the folder.

The newly added node_modules target is purely for convenience/testing, by the way.

@silverwind silverwind changed the title Make node_modules a regular prerequisite again Add NODE_MODULES_EVIDENCE to avoid skipping node_modules rebuilds Mar 2, 2020
@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 Mar 2, 2020
@silverwind
Copy link
Member Author

Still investigating how those broken fomantic builds came to be exactly. https://try.gitea.io/fomantic/semantic.min.css indicates 2.8.4 which means my theory might not have been correct.

@codecov-io
Copy link

codecov-io commented Mar 2, 2020

Codecov Report

Merging #10576 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10576      +/-   ##
==========================================
+ Coverage   43.72%   43.75%   +0.02%     
==========================================
  Files         585      585              
  Lines       82026    82026              
==========================================
+ Hits        35864    35887      +23     
+ Misses      41724    41706      -18     
+ Partials     4438     4433       -5
Impacted Files Coverage Δ
modules/indexer/stats/db.go 50% <0%> (-9.38%) ⬇️
models/unit.go 37.03% <0%> (-2.47%) ⬇️
modules/queue/workerpool.go 56.93% <0%> (-1.07%) ⬇️
models/notification.go 64.71% <0%> (+0.83%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.76% <0%> (+1.92%) ⬆️
services/pull/temp_repo.go 31.62% <0%> (+2.56%) ⬆️
services/pull/patch.go 64.51% <0%> (+2.58%) ⬆️
modules/git/command.go 89.56% <0%> (+2.6%) ⬆️
modules/process/manager.go 78.31% <0%> (+3.61%) ⬆️
services/pull/check.go 56.7% <0%> (+6.7%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa4a297...16d32ad. Read the comment docs.

@silverwind silverwind changed the title Add NODE_MODULES_EVIDENCE to avoid skipping node_modules rebuilds Frontend build improvements Mar 2, 2020
@silverwind
Copy link
Member Author

silverwind commented Mar 2, 2020

I think this is now ready. Here is what it now does:

  • add package-lock.json to webpack/fomantic prereqs making them always rebuild when dependencies change.
  • remove FOMANTIC_EVIDENCE. It seemed better to just track a few output files instead.
  • delete fomantic output files before build to prevent possible bugs in fomantic's build.
  • resolve WEBPACK_SOURCES only once for performance
  • reorder variables for clarity

Regarding the UI issues, I'm still not sure what their cause is but it is certainly either something about unclean builds. I did observe that semantic.min.css file was missing like 30kB data when the issue showed, maybe caused by a bug in fomantic's build process, but the removal of output files before the build should help.

Makefile Show resolved Hide resolved
@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 Mar 3, 2020
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Mar 3, 2020
@lafriks lafriks added this to the 1.12.0 milestone Mar 3, 2020
@silverwind
Copy link
Member Author

silverwind commented Mar 3, 2020

If this doesn't solve the UI issues reliably, the only explanation I have left is that it's an intermittent issue in fomantic 2.8.4 and we should downgrade to 2.8.3. The incomplete fomantic builds don't show on every build, try.gitea.io was affected yesterday, but is okay today.

@silverwind silverwind force-pushed the fix-make-modules branch 2 times, most recently from 09fc4e7 to a135f1d Compare March 3, 2020 18:42
@silverwind silverwind changed the title Frontend build improvements Frontend build improvements, hopefully fix Fomantic build Mar 3, 2020
@silverwind
Copy link
Member Author

Squashed and updated commit message.

- add package-lock.json to webpack/fomantic prereqs making them always rebuild when dependencies change.
- remove FOMANTIC_EVIDENCE. It seemed better to just track a few output files instead.
- delete fomantic output files before build to prevent possible bugs in fomantic's build.
- resolve WEBPACK_SOURCES only once for performance
- reorder variables for clarity
- use clean-all in Dockerfile
- detect busybox for find syntax

Fixes: go-gitea#10569
Fixes: go-gitea#10565
Fixes: go-gitea#10570
Fixes: go-gitea#10568
@silverwind
Copy link
Member Author

Two more fixups included:

  • Use clean-all in Dockerfile so fomantic gets rebuilt in there as well
  • Detect busybox to avoid find: unrecognized: -regextype error

@jolheiser jolheiser merged commit fa6ea60 into go-gitea:master Mar 3, 2020
@silverwind silverwind deleted the fix-make-modules branch March 3, 2020 20:03
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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
8 participants