-
Notifications
You must be signed in to change notification settings - Fork 240
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
Include web interface sources in dist #1439
base: master
Are you sure you want to change the base?
Conversation
@ejurgensen If you find the time, it would be good to know, if this approach to include the web ui sources into the distribution tar and to build the web ui as part of the release process is OK. If it is I would add some documentation and will look into doing some checks in configure.ac (checking if node is installed in case there are no pre-built web ui files available). |
Sure, I'll take a look at it. I will also try to test on a few platforms. I'm impressed you were able do this, autotools is a bit of jungle. |
The objective here is great, but having looked a bit into this has left me conflicted. On one hand, I think it is a major problem that it breaks the normal autoreconf + configure + make. That procedure is so standard that for package building, "make" is in many cases not directly called. See e.g. the OpenWrt package recipe and the Debian package rules. So it will require hacky stuff to make the package build run "make web-build" first, if it is even possible. On the other hand, you have studied the alternatives, and couldn't find anything better, so what then? I don't know much about npm, but to my limited understanding the principle is a bit the same as the other build tools, meaning that like gperf, bison and flex it builds intermediary source. I don't know how they deal with VPATH, but is there a chance that npm could be invoked in the same manner as the other build tools? Another comment is that configure should check for npm/node, but of course only if the web hasn't been built (as it would be in a release dist). Same concept as with gperf/bison/flex. |
I searched quite extensively for solutions and unfortunately this is the best that I found. It is possible to build the web interface with I want to highlight, that the The only other alternative approach I see, is to move the web interface into a project/package and distribute it independently (without autotools). |
Yes, I know, but that is also a relevant use case for package building, especially since it is nice to test that packages will build before making a release. And of course my RPi package is from git, like some of the earlier OpenWrt forked-daapd ones also were. |
I Therefor I think we have three options:
I don't feel comfortable with keeping the current way of building the web ui for several reasons:
I still prefer the solution presented in this PR. I think we can "solve" (or at least mitigate) the issues you mentioned:
This is an ugly/annoying problem and I don't see a perfect solution (and I already spent way to many hours looking for one :-) ... I don't feel comfortable with the current approach (me building the web ui). And strongly believe that this PR is a big improvement over it. |
Yes, I certainly agree with the motivation behind the PR. Would it be an option to use gh actions to automatically invoke I really would like to keep packaging free to build from trunk, or a particular commit, not just from releases. |
@chme, what do you think about my suggestion to automate the building with gh actions? I still think this PR would be excellent to have, so I hope we can find some solution. |
@ejurgensen I am not a fan of automatic rebuilds of the web interface with automatic commits (by a technical user) after a merge. Having the prebuilt web interface files in the git source tree, is in my opinion not good (we don't do this for gperf). It leads to annoying problems, like contributors adding them to PRs. This in turn leads to merge conflicts, requiring to rebase/resolve the conflicts. Additionally our commit history will contain inconsistent state, with a prebuilt web interface that does not match the sources. If the web interface is part of owntone-server the build process is not a standard configure/make. If a package should be built from git, the packaging - in my opinion - has to deal with that. As mentioned, we should offer utility scripts that will help with that as much as possible. |
Yes, good points, I think you are right that it would cause problems. You mentioned before moving the web to a separate project, maybe that is then a way forward? How do you see the integration with owntone-server would then work? I assume we would then have a "owntone-web" repo, where each release (with the help of gh actions?) would produce an asset having the built web UI?
Apart from being non-standard, I think there would be issues with package building. It's not an option to create tags/releases for each of my RPi builds, because my release process consists of building "release candidate" packages from commits (sometimes from branches) until I think it works well enough to become an RPi release. |
Creating tags for release candidates is something a lot of projects do. Even creating tags for feature branches is not uncommon. Having tags and a release tar would allow other packagers to offer something similar you do for RPi.
Yes, something like this, I guess. A release of the web interface would have a source tar and a dist tar (with the built web interface files). |
I had a brief look at a transmission, as it is the project I know of that is most similar. It consists of a daemon, a web ui and cli client. I see they have an "all-in-one" project , so that made me less enthusiastic about the idea of separating the web part. I think it would be best having it all together. So you have me convinced that this approach is the best. I'll have to figure out how to adapt the packaging. Before you merge it, I'd like to make some tests with the Debian package building, i.e. modify my current scripts. What do you think should be the outcome if the user doesn't run 'make web-build'? Should we build without web, or fail (in some nice way)? |
web interface is enabled)
I think, we should fail with some helpful error message. To achieve this, I have added checks in
If no prebuilt web interface is found, configure fails with:
If building with web interface is enabled and no prebuilt web interface exists, running
Note that I shortened the target name to build the web interface from I also added a simple I will look into updating the documentation for building from source. |
I tried modifying my gh action debian package builder to build from chme:web/makefile, and using override_dh_auto_build I can get it to run "make web" before make. However, npm fails with the error you can see here: "npm ERR! request to https://registry.npmjs.org/yaml-eslint-parser/-/yaml-eslint-parser-0.3.2.tgz failed, reason: getaddrinfo EAI_AGAIN registry.npmjs.org". I could download manually just fine. Not sure if it is gh action limitation? I'll investigate more later, but let me know if you have an idea. |
My problem above seems to come from running npm within the pbuilder environment. Instead of doing that I am now trying a two-step approach, where the web UI gets built first and saved as an gh artifact, and then that can be downloaded into the pbuilder environment. This should be more efficient, since it means that installing npm/nodejs, plus building the web, is only done once instead of for each environment. It also keeps the pbuilder actions more similar to building from a release tarball. One thing kind of bugs me, though. "make web" requires "./configure" to have been run, and configure of course checks all dependencies. However, with the above two-step procedure, that means I have to install a lot of packages at the first step that I won't be needing. It would be nice if it was possible to run "make web" in an environment that just has npm/nodejs, but not ffmpeg and all that. I guess that would require a "./configure --only-web" or something like that. Would that be possible? |
This is another attempt to include the web interface source in the distribution tar file and to include the web interface build as part of the building from source process.
Due to npm builds not being able to do a VPATH build (build were the build and source directory are different), I don't think it is possible to fully include the web interface build in the "normal"
autoreconf -i & configure & make
flow.(at least this is my conclusion after searching for solutions)
This PR tries to "solve" it by adding a
Makefile.am
to theweb-src
directory that adds the sources to theEXTRA_DIST
variable to make sure they are included in the distribution.The
Makefile.am
includes additional make targets to build the web interface.To build OwnTone from source with the web interface you will need to run the following commands:
The
make web
requires npm to be installed and will trigger anpm run build
. The web interface output is unchanged (/htdocs
), but I have removed the prebuilt web interface from git.I also added an additional make target to create a tar file of the htdocs folder (maybe this could be added to the github release to allow manually installing the web interface):
Todos:
Ref #962 #552