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 5 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. All files of the web interface are now listed in Makefile,am, to mitigate the risk of adding new files without adding them in Makefile.am a make web-check goal validates that no "unknown" files exists (added to the CI action).

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 2 times, most recently from 0d6921c to f5b50ef Compare January 26, 2025 14:12
@chme
Copy link
Collaborator Author

chme commented Jan 26, 2025

Rebased and squashed commits. The FreeBSD issue was solved by the recent dependency update for the web UI. The FreeBSD action now successfully builds the web UI.

I added a make web-check that checks if all files in web-src/public and web-src/src are listed in the Makefile.am file, to mitigate the risk of accidentally forgetting to add a new file to Makefile.am.

Something like chme/owntone-container@0e40273 can be used to update the Dockerfile in owntone-container.

Let me know, if something else needs to be done. At the moment, I think, this is good to be merged.

@ejurgensen
Copy link
Member

Sounds great. I would like to take this for a test run, I will try to do that asap.

@ejurgensen
Copy link
Member

I tried building a distribution file from this branch, unpacked it in another dir where I ran ./configure and make. Even though the htdocs dir was populated with prebuilt files, the npm build was still started? Maybe I misunderstood: "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."

Is it possible to use the prebuilt files on a system where npm is present? I tried with --disable-webinterface, but then it seemed the webinterface is skipped entirely (make install doesn't install it).

@ejurgensen
Copy link
Member

ejurgensen commented Jan 26, 2025

Some more observations:
autoreconf -vi && ./configure --disable-webinterface && make distcheck
-> fails with:

config.status: error: cannot find input file: `web-src/Makefile.in'
make: *** [Makefile:887: distcheck] Error 1

It also leaves the build dir in a state where there's a bunch of files in owntone-28.11 that can't easily be cleaned up: git clean -x -d -f -> a bunch of permission denied due to the files being read-only. make clean didn't even try to remove them?

Note that with current master this works as expected (the configure flag doesn't impact make distcheck): autoreconf -vi && ./configure --disable-spotify && make distcheck, but in the branch it fails with:

make[2]: Entering directory '/home/espen/build/github/owchme/owntone-28.11/_build/sub'
Making all in src/inputs/librespot-c
/bin/bash: line 21: cd: src/inputs/librespot-c: No such file or directory
make[2]: *** [Makefile:670: all-recursive] Error 1
make[2]: Leaving directory '/home/espen/build/github/owchme/owntone-28.11/_build/sub'
make[1]: *** [Makefile:512: all] Error 2
make[1]: Leaving directory '/home/espen/build/github/owchme/owntone-28.11/_build/sub'
make: *** [Makefile:887: distcheck] Error 1

@chme
Copy link
Collaborator Author

chme commented Jan 27, 2025

I tried building a distribution file from this branch, unpacked it in another dir where I ran ./configure and make. Even though the htdocs dir was populated with prebuilt files, the npm build was still started? Maybe I misunderstood: "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."

Is it possible to use the prebuilt files on a system where npm is present? I tried with --disable-webinterface, but then it seemed the webinterface is skipped entirely (make install doesn't install it).

The first check is if npm is available, if yes, then the web UI is always built and prebuilt files are ignored.
My guess is, that it is preferred to build everything from source and using the prebuilt files is just a fallback.

Should there be a configure flag the will skip building the web UI if prebuilt files are available? I guess, that should be doable ...

Some more observations: autoreconf -vi && ./configure --disable-webinterface && make distcheck -> fails with:

config.status: error: cannot find input file: `web-src/Makefile.in'
make: *** [Makefile:887: distcheck] Error 1

I added commit 1e56bbd that fixes it.
There is some autotools magic at work, that I do not quite understand ... but defining DIST_SUBDIRS was copied from #1439. With having the web UI build integrated it, defining DIST_SUBDIRS is not necessary and causes the issue you noticed.

@ejurgensen
Copy link
Member

Should there be a configure flag the will skip building the web UI if prebuilt files are available?

I think it would be best if the prebuilt files are used if present. Of course, if the source files change, rebuild. That's the way the files generated by bison/flex/gperf work. That means that it will work without modification on buildbots with limited/no internet access, and it should also make the builds deterministic.

@chme
Copy link
Collaborator Author

chme commented Jan 28, 2025

I think it would be best if the prebuilt files are used if present. Of course, if the source files change, rebuild. That's the way the files generated by bison/flex/gperf work.

I don't believe, that this possible. The npm build has no way of knowing, if the existing htdocs are matching the source or not. Especially as not all sources are available (node_modules with all the dependencies is missing), I think, that is the main difference to bison/flex/gperf.
If you know, how this could be achieved, I am all ears :-)

That means that it will work without modification on buildbots with limited/no internet access,

Given that the npm build does not really fit well into the autotools autoreconf/configure/make build system, I fear it is highly unlikely, that it can be done without any modifications for every usecase.

I have added a somewhat experimental commit a94bf97, that adds a --with-prebuilt-webinterface configure flag. If that flag is given to configure, then no npm build is triggered (even if npm is available) and only the prebuilt web interface is used.
This would at least give the option to skip the npm build, if there is no or only limited internet access.

@ejurgensen
Copy link
Member

The npm build has no way of knowing, if the existing htdocs are matching the source or not

I think make just does this by comparing the target's file timestamp with the source's

@chme chme force-pushed the chore/build-web-autotools branch from a94bf97 to c181a78 Compare February 3, 2025 18:49
@chme
Copy link
Collaborator Author

chme commented Feb 3, 2025

c181a78 is a proof of concept commit. It moves preparing the build dir for the web UI from web-src/Makefile.am to configure.ac:

  • The advantage is, that the web UI build is only done, if the web-src sources changed compared to the prebuilt htdocs (if it is available).
  • The disadvantage is, that if the web UI needs to be build, it always has to run npm clean-install to download the dependencies in node_modules.

@ejurgensen, I'll try to summarize what I identified as the main difficulties and the two solutions I see. If you agree with one of them, I can try to finish this PR.


Web UI build restrictions

The web UI build has some restrictions, that make it hard to integrate into an autotools based build:

  1. The distribution of the web UI sources do not contain the third party libraries (in node_modules). These libraries/dependencies have to be downloaded by running npm clean-install (or npm install) in order to build the web UI.
  2. The web UI build with npm run build has to be done from the source folder (or at least a folder that has all sources in it). It is not possible to run the build from a build dir and tell it the sources are in a different folder.
    • It seems also not possible to just link the source files into the build dir (I only did a short test for this and the build failed - did not spent to much time trying this, so there might be a way to get it working, though I am not sure if this will really help)

Due to these restrictions and from what I tested, I am convinced, that there will be no perfect solution.
But I think, the remaining issues or inconveniences are acceptable, given that they allow to distribute the full source (with the exception of the third party libraries in node_modules).

For now I see two solutions that are - from my point of view - both acceptable.

Solution 1

  • Makefile.am makes sure, that the sources from web-src are in the build dir. If they are missing ("out of tree" build), it copies the files into the build dir.
  • If npm is available, make will build the web UI, ignoring available prebuilt htdocs.
    • To force using prebuilt htdocs a configure flag has to be set.
  • If npm is not available, an existing prebuilt htdocs will be used.

A simpified / incomplete Makefile.am looks like:

node_modules: package.json package-lock.json
    # Test if builddir != srcdir
	#   If true, copy source files to builddir
	npm clean-install

htdocs: node_modules $(WEB_SOURCE_FILES)
	npm run build

all-local: htdocs

Pros;

  • Only runs npm clean-install if dependency config changes (package*.json).

Cons;

  • When npm is available, always builds htdocs even if prebuilt files exist (and match the source).
    • Happens because node_modules is missing.

Solution 2

  • configure.ac makes sure, that the sources are in the build dir.
  • Build of the web UI only depends on the owntone sources (and not on node_modules).
  • node_modules is downloaded as part of the web UI build.

configure.ac

AC_CONFIG_COMMANDS([websrc],
	[
		# Test if builddir != srcdir
		#   If true, copy source files to builddir
		...
	],[])
AC_CONFIG_COMMANDS([],
	[
		# Test if builddir != srcdir
		#   If true, copy prebuilt files to builddir/htdocs
		...
	],[])

Makefile.am

htdocs: $(WEB_SOURCE_FILES)
	npm clean-install
	npm run build

Pros:

  • web UI build only executed if source files changes and do not match prebuilt htdocs.
    • This works because the build only depends on the distributed sources.

Cons:

  • Runs npm clean-install if a web-src source file changes - not only when package*.json changes.
    • In my opinion, this is acceptable. During web UI development, normally a dev server is started once and build is not done via make.

@ejurgensen
Copy link
Member

I don't know about moving all that stuff into configure.ac, don't you think that becomes pretty ugly?

It seems like twisting autotools quite a lot. Autotools is already difficult, but this would make it even more more difficult to maintain. Such a change could be justified by solving a major problem that we have now, but I'm not sure that's the case. While it would be nice to support normal "make", the current solution with prebuilt by github isn't giving much trouble in my view.

The Makefile.am solution seems more clean, but still has the problem with not supporting use of the prebuilt files (except with a special flag), right?

Do you think this is really an autotools problem - i.e. would it be different if we used cmake or meson?

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