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

Include web interface sources in dist #1439

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

Conversation

chme
Copy link
Collaborator

@chme chme commented Apr 2, 2022

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 the web-src directory that adds the sources to the EXTRA_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:

autoreconf -i
./configure
make web
make

The make web requires npm to be installed and will trigger a npm 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):

make web-dist

Todos:

  • Update install documentation

Ref #962 #552

@chme
Copy link
Collaborator Author

chme commented May 7, 2022

@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).

@ejurgensen
Copy link
Member

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.

@ejurgensen
Copy link
Member

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.

@chme
Copy link
Collaborator Author

chme commented May 14, 2022

On the other hand, you have studied the alternatives, and couldn't find anything better, so what then?

I searched quite extensively for solutions and unfortunately this is the best that I found.

It is possible to build the web interface with configure && make but due to npm not supporting VPATH, this will break at least for make dist-check. And this is probably also not acceptable for package maintainers.

I want to highlight, that the make web-build should only be necessary when building from the git sources. It should be called as part of the release tar ball creation. When building from a release tar ball, the web interface is already built and the standard configure && make works like it does now (therefore the OpenWrt package recipe should not require any changes with this PR).

The only other alternative approach I see, is to move the web interface into a project/package and distribute it independently (without autotools).

@ejurgensen
Copy link
Member

I want to highlight, that the make web-build should only be necessary when building from the git sources

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.

@chme
Copy link
Collaborator Author

chme commented May 22, 2022

I wasted spent some additional hours attempting to find a solution. But npm is simply not made for VPATH / out-of-tree builds. I cannot find any other project that builds a c project and includes the npm build of a web ui in the Makefile.

Therefor I think we have three options:

  • Someone (other than me) steps up and presents a solution
  • We remove the web ui from owntone-server and provide it as a separate package to install
  • We adopt the solution presented in this PR (building the web interface outside of the standard configure-make commands)

I don't feel comfortable with keeping the current way of building the web ui for several reasons:

  • We currently include the web ui built from my local machine in our releases
  • Contributions to the web ui require either to accept that a web ui build is done on the contributors machine (and included in the PR) or me to do a web ui build from my machine and push it separately
  • We don't include the sources for the ui in the release tar ball, thus it is not possible to build the web ui from it

I still prefer the solution presented in this PR. I think we can "solve" (or at least mitigate) the issues you mentioned:

  • In my opinion it would be good to have also tags/releases for the RPi builds. These could be beta build tags, for which we then create a release tar ball
  • Creation of a release could be automated with gh-actions
  • The gh-action to tag/build a release could do some additional checks before actually creating the tag/release
  • We can provide a simple shell script that would do the whole build (with autoreconf, ./configure, make web-build, make) to make building from git easier
  • As you (and I) mention in the previous comments, additional check in configure.ac should test for availability of npm. And probably it is also possible to output helpful error messages during make if make web-build was not executed and inclusion of the web ui was configured

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.

@ejurgensen
Copy link
Member

Yes, I certainly agree with the motivation behind the PR. Would it be an option to use gh actions to automatically invoke make web-build (I guess triggered by any push to websrc?) and then make actions push new/changed output to the htdocs tree? Basically doing what you are doing right now on your computer.

I really would like to keep packaging free to build from trunk, or a particular commit, not just from releases.

@ejurgensen
Copy link
Member

@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.

@chme
Copy link
Collaborator Author

chme commented Jun 15, 2022

@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.
Same inconsistency can appear, if for some reason the gh action fails (e. g. downloading the npm dependencies fails due to network errors).

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.

@ejurgensen
Copy link
Member

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?

As mentioned, we should offer utility scripts that will help with that as much as possible.

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.

@chme
Copy link
Collaborator Author

chme commented Jun 15, 2022

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.
(Would also make it transparent from which sources a binary was built)

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?

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).

@ejurgensen
Copy link
Member

ejurgensen commented Jun 16, 2022

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)?

@chme
Copy link
Collaborator Author

chme commented Jun 18, 2022

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)?

I think, we should fail with some helpful error message.

To achieve this, I have added checks in configure.ac and in htdocs/Makefile.am.

configure.ac now checks if the npm program is available if building the web interface is enabled.
If npm is not available but a prebuilt web interface exists, it prints an info message:

npm not found, but a prebuilt web interface seems to be present.
If you modify any web interface files, you will need to install it.

If no prebuilt web interface is found, configure fails with:

npm required, please install it.

If building with web interface is enabled and no prebuilt web interface exists, running make without running make web results in the error message Web interface files are missing! Did you run "make web"?.
Following the relevant part of the output from make:

Making all in htdocs
make[2]: Entering directory '/home/owntone/htdocs'
Makefile:623: *** Web interface files are missing! Did you run "make web"?.  Stop.
make[2]: Leaving directory '/home/owntone/htdocs'
make[1]: *** [Makefile:666: all-recursive] Error 1
make[1]: Leaving directory '/home/owntone'
make: *** [Makefile:508: all] Error 2

Note that I shortened the target name to build the web interface from web-build to just web (less to type :-)).

I also added a simple build.sh shell script that does the autoreconf/configure/make web/make. Not sure if this is helpful (can drop it if you think it is not necessary).

I will look into updating the documentation for building from source.
Let me know what you think can/should be improved.

@ejurgensen
Copy link
Member

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.

@ejurgensen
Copy link
Member

ejurgensen commented Jun 24, 2022

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?

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