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

[web-src] Include web interface sources in distribution files; build web interface as part of autotools #1838

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

chme
Copy link
Collaborator

@chme chme commented Jan 18, 2025

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:

autoreconf -i
./configure
make

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 from htdocs to web-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.

    • In configure.ac a check for the presence of the npm binary is done.
    • If it is not present, another test is done whether prebuilt web interface files (htdocs) are available.
    • If prebuilt files are available, only a message is printed that the web interface will not be built and the prebuilt files will be used. Otherwise, configure fails.
  • web-src/Makefile replaces the htdocs/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

  • The build of the web interface as part of autotools assumes a clean build. At the moment, it will not detect if the web interface source files changed between builds. To "fix" this, it would be necessary to list all source files (instead of only the "src" folder) in WEB_FILES.

Testing

Some tests I did to validate this approach:

  • make distcheck runs fine (on my system).
  • The distribution tar file contains the web interface sources and the htdocs build.
  • Building from the tar file without installed npm, will use and install the prebuilt htdocs files.
  • Building on a system without npm is possible with passing 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

@chme chme mentioned this pull request Jan 18, 2025
1 task
@chme chme force-pushed the chore/build-web-autotools branch from 5b2da4d to 9581a3e Compare January 19, 2025 06:38
@chme
Copy link
Collaborator Author

chme commented Jan 19, 2025

mmh ... the FreeBSD npm build of the web interface is failing (see log from CI below).
I guess, this is unrelated to the changes in this PR and building the web interface with FreeBSD generally fails?
If this is true, would it be OK to disable the web interface build in the FreeBSD CI action?

  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

@chme
Copy link
Collaborator Author

chme commented Jan 19, 2025

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 Makefile.am. Forgetting to list it might not be noticed directly, as it will only effect the distribution tar file (and out-of-tree builds).

@ejurgensen
Copy link
Member

ejurgensen commented Jan 19, 2025

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.

@chme
Copy link
Collaborator Author

chme commented Jan 19, 2025

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 master the web interface is not built in the FreeBSD build. This is only happening now, because of the changes in this PR.

It would be nice if there was some "linter" that could check. If not, maybe we could hack together a github action that checks?

Yes, that makes sense. I'll look into this, maybe it can be included in the current web linter action.

@chme chme force-pushed the chore/build-web-autotools branch from 81ce9bc to e4ac74c Compare January 19, 2025 11:30
@chme
Copy link
Collaborator Author

chme commented Jan 19, 2025

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.
Another option would be to leave the webui action untouched and call this goal in the "ubuntu" workflow, where all dependencies are already present ...

@chme chme force-pushed the chore/build-web-autotools branch from e4ac74c to 0d6921c Compare January 19, 2025 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants