-
Notifications
You must be signed in to change notification settings - Fork 242
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
[web-src] Include web interface sources in distribution files; build web interface as part of autotools #1838
base: master
Are you sure you want to change the base?
Conversation
5b2da4d
to
9581a3e
Compare
mmh ... the FreeBSD npm build of the web interface is failing (see log from CI below). gmake[2]: Entering directory '/home/runner/work/owntone-server/owntone-server/web-src'
/usr/local/bin/npm run build
> [email protected] build
> vite build --base='./'
/home/runner/work/owntone-server/owntone-server/web-src/node_modules/rollup/dist/native.js:84
throw new Error(
^
Error: Your current platform "freebsd" and architecture "x64" combination is not yet supported by the native Rollup build. Please use the WASM build "@rollup/wasm-node" instead.
The following platform-architecture combinations are supported:
android-arm
android-arm64
darwin-arm64
darwin-x64
linux-arm
linux-arm (musl)
linux-arm64
linux-arm64 (musl)
linux-ppc64
linux-riscv64
linux-s390x
linux-x64
linux-x64 (musl)
win32-arm64
win32-ia32
win32-x64
If this is important to you, please consider supporting Rollup to make a native build for your platform and architecture available.
at throwUnsupportedError (/home/runner/work/owntone-server/owntone-server/web-src/node_modules/rollup/dist/native.js:84:8)
at getPackageBase (/home/runner/work/owntone-server/owntone-server/web-src/node_modules/rollup/dist/native.js:75:3)
at Object.<anonymous> (/home/runner/work/owntone-server/owntone-server/web-src/node_modules/rollup/dist/native.js:37:21)
at Module._compile (node:internal/modules/cjs/loader:1565:14)
at Object..js (node:internal/modules/cjs/loader:1708:10)
at Module.load (node:internal/modules/cjs/loader:1318:32)
at Function._load (node:internal/modules/cjs/loader:1128:12)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:219:24)
at cjsLoader (node:internal/modules/esm/translators:263:5)
Node.js v22.12.0
gmake[2]: Leaving directory '/home/runner/work/owntone-server/owntone-server/web-src'
gmake[2]: *** [Makefile:560: htdocs] Error 1
gmake[1]: Leaving directory '/home/runner/work/owntone-server/owntone-server'
gmake[1]: *** [Makefile:675: install-recursive] Error 1
gmake: *** [Makefile:980: install] Error 2 |
Giving it some thought, explicitly listing the web interface source files might not be a bad idea. They rarely change and having autotools properly detect whether a rebuild of the web interface is necessary is good to have. See 15b7e0d Downside is, that whenever a file is added to the web interface sources, it has to be added to the list of files in |
I haven't looked at the implementation, but I think it sounds very promising from your description of it. It sounds like it will work in all normal build scenarios, and keeps the ability to distribute prebuilt htdocs. I agree that listing all the files in Makefile.am so it can catch updates is preferable. The problem about forgetting to update is of course very real, I think that risk also exists in some cases for the C-source Makefiles (maybe the header files?). It would be nice if there was some "linter" that could check. If not, maybe we could hack together a github action that checks? Regarding FreeBSD I just now triggered a re-run on master to see if it started failing there. |
You probably won't see it failing in the GitHub-CI action/workflow. In
Yes, that makes sense. I'll look into this, maybe it can be included in the current web linter action. |
81ce9bc
to
e4ac74c
Compare
The FreeBSD issue should be fixable by updating the dependencies (especially rollup to > v4.24.1): rollup/rollup#5491. I'll do an update and check if it is now building on FreeBSD. I also extended the "web-lint" make goal and changed the github action to use it. Unfortunately, this required installing a bunch of dependencies ... I copied the install from the "ubuntu" workflow. |
…mat, update, serve)
Checks with "make web-check" that all web ui source files are distributed as part of the release tar.
e4ac74c
to
0d6921c
Compare
I thought, I tackle again the issue with the web interface build integration (and source distribution) into the autotools build chain ... and so far, this is the most promising solution :-)
The solution presented in this PR is a bit different from previous attempts.
It does not require special
make
goals to be run for the web interface. Instead the web interface is built as part of the "normal" build process:The prebuilt htdocs files are deleted from git (and removed from
configure.ac
,Makefile.am
). They are still part of the distribution tar file - but the location changed fromhtdocs
toweb-src/htdocs
.Building without the web interface by passing "--disable-webinterface" to configure is still possible.
configure.ac
Check build requirements for building the web interface (similar to what was done in Include web interface sources in dist #1439.
configure.ac
a check for the presence of the npm binary is done.configure
fails.web-src/Makefile
replaces thehtdocs/Makefile
in AC_CONFIG_FILES.web-src/Makefile.am
Web interface build is triggered with the
all-local
make goal.To support out-of-tree-builds (like it is done during
make distcheck
), the build checks first if source and build dirs are the same. If not the source files are copied into the build dir. And the build is done in the build dir afterwards (with installing node_modules etc.).Installation of the web interface files is done in
install-data-local
.Known Issues
WEB_FILES
.Testing
Some tests I did to validate this approach:
make distcheck
runs fine (on my system).disable-webinterface
.There is probably a bunch of stuff, I did not think of, that might break with these changes. Let me know if this is worth pursuing further.
Ref #1439 #962 #552