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

[htdocs/web-src] Include build of htdocs files into configure/make #962

Closed
wants to merge 7 commits into from

Conversation

chme
Copy link
Collaborator

@chme chme commented Apr 26, 2020

@ejurgensen this is my attempt at including the build of the web interface into the forked-daapd configure/make build.

I have to say, that my Makefile/autotools knowledge is really limited and i also did not find good examples that already have an npm built web interface included into the autotools toolchain.

What this PR does is:

  • New Makefile in web-src that is included in the root Makefile.am
  • If npm is installed on the build system, build the web interface as part of make
  • If npm is not installed, building the web interface is a noop
  • Adds a new target make dist-web that creates a forked-daapd-htdocs-VERSION.tar.gz file for the web interface dist files
  • Add the web interface source and dist files to the forked-daapd dist file (make dist)
  • Removes the old admin interface

There is probably a lot, that i am doing wrong ... so i would appreciate, if you can take a look at the changes and tell me if this is something we want and if yes, what is missing and needs to be changed/added/removed.

@ejurgensen
Copy link
Member

Sounds very good to me. Autotools is black magic to me, so I don't know how much I can help, but I will take a look at it and also test it on a few platforms.

@ejurgensen
Copy link
Member

Here's a bit of feedback on this after I looked at a few "special" platforms.

OpenWrt:
I maintain this package, and it currently includes the web interface. The way it works is that their build server downloads the release xz from github (which I make with "make dist-xz"), and then runs pretty standard ./configure and make. The build server does not have antlr, gperf and I guess no npm either. So I'm thinking how I would do this after this change? When I release forked-daapd, I could do "make dist-xz" and "make dist-web" and then publish both assets here on github. But it would make it more difficult, because then the OpenWrt build script has to combine the two. I don't really have a solution for this, so now I am just bringing it up.

FreeBSD 12.1:
This change seems to partially break gmake -> it builds fine (with npm installed), but "gmake install" now only installs the web files, and "gmake clean" doesn't work any more (I didn't test "make clean" on other platforms).

It would also be good if we could somehow get it tested on MacOS.

Regarding the Makefile itself, shouldn't "cp" be "$(INSTALL)" or something like that?

@chme
Copy link
Collaborator Author

chme commented May 9, 2020

OpenWrt:
I maintain this package, and it currently includes the web interface. The way it works is that their build server downloads the release xz from github (which I make with "make dist-xz"), and then runs pretty standard ./configure and make. The build server does not have antlr, gperf and I guess no npm either. So I'm thinking how I would do this after this change? When I release forked-daapd, I could do "make dist-xz" and "make dist-web" and then publish both assets here on github. But it would make it more difficult, because then the OpenWrt build script has to combine the two. I don't really have a solution for this, so now I am just bringing it up.

The result of make dist-xz already includes the build web interface files (located under /web-src/dist, the dist-hook target takes care of that). Calling make install should work (the Makefile is included as well and will take care of installing the files to the htdocs directory).
The make dist-web is only for convenience, if a separate distribution file only for the web interface stuff should be created.

FreeBSD 12.1:
This change seems to partially break gmake -> it builds fine (with npm installed), but "gmake install" now only installs the web files, and "gmake clean" doesn't work any more (I didn't test "make clean" on other platforms).

Will have to try this out see what is missing/wrong ... will report back.

Regarding the Makefile itself, shouldn't "cp" be "$(INSTALL)" or something like that?

I honestly don't know, what the right approach is. I will try with "$(INSTALL)".

@ejurgensen
Copy link
Member

The result of make dist-xz already includes the build web interface files

Ok maybe my understanding of how it works is lacking then - when I run "make dist-xz" then npm is not invoked (unlike gperf and antlr), so instead the web interface (which is otherwise the output of npm) is copied from /web-src/dist? Is that correct? If so, I'm thinking it could be a hassle to keep /web-src/dist in sync with the source files, since every time you make a change you have to make sure the dist directory is also updated?

chme added 3 commits May 16, 2020 07:55
Add a Makefile into web-src that takes care of building the forked-daapd
player web interface. It only builds the web interface if the npm binary
is installed on the build system. Otherwise the prebuild files under the
dist directory are used.
@chme
Copy link
Collaborator Author

chme commented May 17, 2020

Did some more trial and error ... and change it to not directly include the Makefile but instead use autoconf to create the web-src/Makefile from the Makefile.in file. It builds for me now on FreeBSD.

This is based on: https://www.gnu.org/software/automake/manual/html_node/Third_002dParty-Makefiles.html (and by looking into the other automake generated Makefile.in files in forked-daapd)

Regarding MacOS, maybe we could use Github actions for this?
And before this could be considered for merge, we should make sure cross compilation still works ... (i'll try to look into this).

If you have some free time @ejurgensen, i'll appreciate if you could give this another try.

@ejurgensen
Copy link
Member

Great work you are doing here, it seems like pretty complex stuff. I've now tried the "web-makefile" branch on FreeBSD and OpenWrt (cross-compilation), unfortunately both failed. Below are the errors.

FreeBSD 12.1, with npm installed (and in a vm I couldn't copy/paste from):
freebsd121
(looks like BSD install doesn't understand the -t argument)

OpenWrt, cross-compilation, no npm installed:

make[7]: Entering directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/src'
 /bin/mkdir -p '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/ipkg-install/usr/sbin'
  /bin/bash ../libtool   --mode=install /usr/bin/install -c forked-daapd '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/ipkg-install/usr/sbin'
OpenWrt-libtool: install: /usr/bin/install -c forked-daapd /home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/ipkg-install/usr/sbin/forked-daapd
make[7]: Nothing to be done for 'install-data-am'.
make[7]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/src'
make[6]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/src'
make[5]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/src'
Making install in web-src
make[5]: Entering directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/web-src'
/bin/mkdir -p /usr/share/forked-daapd/htdocs
/bin/mkdir: cannot create directory '/usr/share/forked-daapd': Permission denied
make[5]: *** [Makefile:104: installdirs] Error 1
make[5]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/web-src'
make[4]: *** [Makefile:630: install-recursive] Error 1
make[4]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1'
make[3]: *** [Makefile:927: install] Error 2
make[3]: Leaving directory '/home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1'
make[2]: *** [Makefile:87: /home/espen/OpenWrt/trunk/build_dir/target-mips_24kc_musl/forked-daapd-27.1/.built] Error 2
make[2]: Leaving directory '/home/espen/OpenWrt/trunk/feeds/packages/sound/forked-daapd'
time: package/feeds/packages/forked-daapd/compile#0.35#0.10#0.53
make[1]: *** [package/Makefile:113: package/feeds/packages/forked-daapd/compile] Error 2
make[1]: Leaving directory '/home/espen/OpenWrt/trunk'
make: *** [/home/espen/OpenWrt/trunk/include/toplevel.mk:227: package/feeds/packages/forked-daapd/compile] Error 2

@chme
Copy link
Collaborator Author

chme commented May 21, 2020

The Makefile.in is getting more and more complex ... not sure if this is a good thing ;-)
I copied the logic for installing data files from the automake generated Makefile.in and i hope this solves the cross compilation issue. At least it did solve gmake install on FreeBSD for me.

I also added some changes to configure.ac to properly detect if the web interface should be built and if all prerequisites are installed. I did some tests with npm installed and not installed and with the web interface enabled/disabled. It seems to work fine ...

@ejurgensen
Copy link
Member

I think using Github actions for MacOS is a good idea, so I tried adding such a workflow file. However, it seems there is some Github bug. It keeps saying that actions are not enabled, even though I did enable them. Looks like other people have the same issue: https://github.community/t5/GitHub-Actions/Unable-to-enable-Actions-for-this-repository/td-p/36260

Maybe it works in your fork?

@chme
Copy link
Collaborator Author

chme commented May 23, 2020

I updated the master branch in my fork and Github actions are working: https://github.com/chme/forked-daapd/actions (though the MacOS build fails with a permission problem installing antlr3)

@ejurgensen
Copy link
Member

Github support was able to help get the actions working, so that is pretty nice.

I tried building the branch again on OpenWrt with no npm, and it fails with:

checking for npm... no
configure: error: npm required for web interface, please install it (or use --disable-webinterface).

But perhaps that was also the intention? I'm unsure what your plan was when building from git and npm isn't present.

If I can make a release with e.g. "make dist-gzip" on a machine with npm, and then build from the tarball on another machine without npm I would think it is fine. Then it would be like antlr3/gperf.

@nguyendj
Copy link

I’m looking forward latest forked-daapd packages release With web interface.
Now mine only server works With no webui, no remote.

@ejurgensen
Copy link
Member

@chme I'm not sure about the status of this PR, but if you think it is ok you are welcome to merge it

@chme
Copy link
Collaborator Author

chme commented Aug 22, 2020

There are still problems I need to fix. make distcheck currently fails.

@chme
Copy link
Collaborator Author

chme commented Apr 2, 2022

Closing in favor of #1439

@chme chme closed this Apr 2, 2022
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.

3 participants