mkdeb.sh.in: pass remaining arguments to ./configure #5154

kmk3 · 2022-05-20T20:08:25Z

Currently, mkdeb.sh (which is used to make a .deb package) runs
./configure with hardcoded options (some of which are automatically
detected based on configure-time variables). To work around the
hardcoding, contrib/fj-mkdeb.py is used to add additional options by
rewriting the actual call to ./configure on mkdeb.sh. For example, the
following invocation adds --disable-firetunnel to mkdeb.sh:

$ ./configure && ./contrib/fj-mkdeb.py --disable-firetunnel

To avoid depending on another script and to avoid re-generating
mkdeb.sh, just let the latter pass the remaining arguments (the first
one is an optional package filename suffix) to ./configure directly.
Example:

$ make distclean && ./configure && make dist &&
  ./mkdeb.sh "" --disable-firetunnel

Additionally, change contrib/fj-mkdeb.py to do roughly the same as the
above example, by simply forwarding the arguments that it receives to
./mkdeb.sh (which then forwards them to ./configure). Also, remove the
--only-fix-mkdeb option, since the script does not change mkdeb.sh
anymore. With these changes, the script's usage (other than when using
--only-fix-mkdeb) should remain the same.

Note: To clean the generated files and then make a .deb package using
the default configuration, the invocation is still the same:

$ make distclean && ./configure && make deb

Note2: Running ./configure in the above examples is only needed for
generating Makefile/mkdeb.sh from Makefile.in/mkdeb.sh.in after running
distclean, so that running make / ./mkdeb.sh afterwards works.

Should fully fix #772.

Relates to #1205 #3414 #5142 #5148.

kmk3 · 2022-05-20T20:13:15Z

Depends on #5142.

reinerh · 2022-05-21T09:24:30Z

Makefile.in

-DISTFILES_TEST = "test/Makefile.in test/apps test/apps-x11 test/apps-x11-xorg test/root test/private-lib test/fnetfilter test/fcopy test/environment test/profiles test/utils test/compile test/filters test/network test/fs test/sysutils test/chroot"
+DISTFILES = \
+COPYING \
+Makefile \


the dist target is creating the tarball that is released after a new firejail version is released.
normally release tarballs don't contain generated files like Makefiles (or mkdeb.sh), as they likely can't be used by someone downloading the tarball (because they contain paths of the system where it was generated, or other configuration options).
(a common exception to that is the pre-generated configure script, though in theory it also would not need to be shipped.)

i would prefer to keep a "clean" release tarball.

I'll look into this.

@reinerh on May 21:

the dist target is creating the tarball that is released after a new
firejail version is released. normally release tarballs don't contain
generated files like Makefiles (or mkdeb.sh), as they likely can't be used by
someone downloading the tarball (because they contain paths of the system
where it was generated, or other configuration options). (a common exception
to that is the pre-generated configure script, though in theory it also
would not need to be shipped.)

i would prefer to keep a "clean" release tarball.

Yes, I don't consider it good either; I just didn't manage to think of
something better when opening this PR. As of the current version, this is not
done anymore.

Related to this, I now see that as of #5142, since the entirety of src/ and
test/ are part of DISTFILES / DISTFILES_TEST (rather than only the exact
files and dirs that are needed), the generated Makefile files in these
directories are also being copied on make dist (besides just the
Makefile.in files). But that should be fixed with the PR that will come
after #5140.

Related to this, I now see that as of #5142, since the entirety of src/ and
test/ are part of DISTFILES / DISTFILES_TEST (rather than only the exact
files and dirs that are needed), the generated Makefile files in these
directories are also being copied on make dist (besides just the
Makefile.in files). But that should be fixed with the PR that will come
after #5140.

Actually, only src/ is affected, as test/Makefile.in is referenced directly in
DISTFILES_TEST and there is no other makefile in test/.

Fixed by #5182.

laniakea64 · 2022-05-24T14:53:40Z

Thanks for the CC. Having first-class support of custom configure options in deb package would be great! 👍

This PR doesn't build though? -

$ ./configure --enable-apparmor --disable-firetunnel
$ make deb

errors out with -

mv: cannot stat 'firejail-0.9.69/debian/usr/share/doc/firejail/RELNOTES': No such file or directory
make: *** [Makefile:239: deb] Error 1

Oh, seems to manually require adding --prefix=/usr. That specific one should be enforced for .deb package, or at least the default for deb package if it's intended that unusual prefixes are now allowed for the .deb package?

lgtm-com · 2022-05-27T21:07:02Z

This pull request introduces 1 alert when merging c59ab8a into 880f2c9 - view on LGTM.com

new alerts:

1 for Unused import

PACKAGE_TARNAME was added on commit 87e7b31 ("Configure Debian package with AA and SELinux options", 2020-05-13) / PR netblue30#3414. TOP was added on commit ed4a24c ("porting make deb-apparmor from LTS build", 2019-01-26).

This (mostly) restores the behavior from before commit 1fb814e ("Makefile.in: stop running distclean on dist", 2022-05-13) / PR netblue30#5142. ./configure still has to be called before calling ./contrib/fj-mkdeb.py (to generate Makefile from Makefile.in before calling `make distclean`).

kmk3 · 2022-05-27T21:45:21Z

@laniakea64 commented on May 24:

Thanks for the CC.

No problem.

Having first-class support of custom configure options in deb package would
be great! +1

Agreed; as of the current branch version, it should be more "first-class" than
before :)

This PR doesn't build though? -
$ ./configure --enable-apparmor --disable-firetunnel
$ make deb
errors out with -
mv: cannot stat 'firejail-0.9.69/debian/usr/share/doc/firejail/RELNOTES': No such file or directory
make: *** [Makefile:239: deb] Error 1
Oh, seems to manually require adding --prefix=/usr. That specific one
should be enforced for .deb package, or at least the default for deb package
if it's intended that unusual prefixes are now allowed for the .deb package?

Indeed, it looks like mkdeb.sh assumes --prefix=/usr (like in the mv
command above).

I can try to make the script work with any prefix, but I'll probably only get
to that after PR #5140 (and the one after that), as I'm already juggling
multiple branches on top of each other.

Makefile.in

Currently, mkdeb.sh (which is used to make a .deb package) runs ./configure with hardcoded options (some of which are automatically detected based on configure-time variables). To work around the hardcoding, contrib/fj-mkdeb.py is used to add additional options by rewriting the actual call to ./configure on mkdeb.sh. For example, the following invocation adds --disable-firetunnel to mkdeb.sh: $ ./configure && ./contrib/fj-mkdeb.py --disable-firetunnel To avoid depending on another script and to avoid re-generating mkdeb.sh, just let the latter pass the remaining arguments (the first one is an optional package filename suffix) to ./configure directly. Example: $ make distclean && ./configure && make dist && ./mkdeb.sh "" --disable-firetunnel Additionally, change contrib/fj-mkdeb.py to do roughly the same as the above example, by simply forwarding the arguments that it receives to ./mkdeb.sh (which then forwards them to ./configure). Also, remove the --only-fix-mkdeb option, since the script does not change mkdeb.sh anymore. With these changes, the script's usage (other than when using --only-fix-mkdeb) should remain the same. Note: To clean the generated files and then make a .deb package using the default configuration, the invocation is still the same: $ make distclean && ./configure && make deb Note2: Running ./configure in the above examples is only needed for generating Makefile/mkdeb.sh from Makefile.in/mkdeb.sh.in after running distclean, so that running `make` / `./mkdeb.sh` afterwards works. Should fully fix netblue30#772. Relates to netblue30#1205 netblue30#3414 netblue30#5148.

Since `make deb-apparmor` already exists, use that for now instead of changing what `make deb` does. This fixes CI. Added on commit 494b26d ("adding --enable-apparmor by default for make deb - most Debian-based distros have apparmor enabled by default", 2022-06-03). Kind of relates to netblue30#5154.

This reverts commit 1fb814e. If distclean is not executed before copying the files on dist, then the generated files inside src/ are included in the dist archive: $ ./configure >/dev/null && make distclean >/dev/null && ./configure >/dev/null && make dist | grep 'Makefile$' | wc -l 26 This happens because src/ is copied wholesale on dist (see DISTFILES). Revert the commit to ensure that only the input files (such as the "Makefile.in" files) are archived. Related discussion: netblue30#5154 (review) Relates to netblue30#5142.

Relates to #5133 #5154.

laniakea64 · 2022-06-09T18:43:41Z

Sorry I couldn't get back to this sooner.

fj-mkdeb.py is broken in 0.9.70 by this PR -

$ ./contrib/fj-mkdeb.py --disable-firetunnel --enable-apparmor
make: *** No rule to make target 'distclean'.  Stop.

That error can be worked around by running ./configure first, but then it produces a deb package named "firejail_0.9.70--disable-firetunnel_1_amd64.deb" with firetunnel support enabled?

$ sudo apt install ./firejail_0.9.70--disable-firetunnel_1_amd64.deb 
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
Note, selecting 'firejail' instead of './firejail_0.9.70--disable-firetunnel_1_amd64.deb'
Suggested packages:
  python
The following NEW packages will be installed:
  firejail
0 upgraded, 1 newly installed, 0 to remove and 0 not upgraded.
...
$ firejail --version
firejail version 0.9.70

Compile time support:
        - always force nonewprivs support is disabled
        - AppArmor support is enabled
        - AppImage support is enabled
        - chroot support is enabled
        - D-BUS proxy support is enabled
        - file transfer support is enabled
        - firetunnel support is enabled
        - IDS support is disabled
        - networking support is enabled
        - output logging is enabled
        - overlayfs support is disabled
        - private-home support is enabled
        - private-cache and tmpfs as user enabled
        - SELinux support is disabled
        - user namespace support is enabled
        - X11 sandboxing support is enabled

The first-class method to build .deb with custom configure options works and does not have these problems. Also it is straightforward enough that fj-mkdeb.py now seems superfluous. Should fj-mkdeb.py just be removed?

I can try to make the script work with any prefix,

Thanks but I think it's not necessary. The only time I've ever needed an alternate prefix .deb was when I had two versions of firejail installed concurrently (the reason is documented on this issue tracker somewhere and is no longer relevant), and I had to change more than just prefix to make that work.

This reverts commit b4d0b24. This amends commit 56b86f8 ("Revert "Makefile.in: stop running distclean on dist"", 2022-06-08) / PR #5182. Since the revert, `make dist` itself already runs `make distclean`. This also means that it is no longer necessary to run ./configure (to generate "Makefile" from "Makefile.in") before running ./contrib/fj-mkdeb.py. Misc: This is not a clean revert. Relates to #5154.

This amends commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR #5154. See also #5176.

Instead of using the first argument as the `EXTRA_VERSION` variable. This should make the usage of mkdeb.sh less confusing, especially when one is not trying to set the variable. As for using `EXTRA_VERSION` (which is still optional with this commit), make sure that it is set as an environment variable before caling mkdeb.sh. Example: env EXTRA_VERSION=-apparmor ./mkdeb.sh --enable-apparmor See also commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR netblue30#5154.

kmk3 · 2023-02-10T22:01:45Z

@laniakea64 commented on Jun 9, 2022:

Sorry I couldn't get back to this sooner.

No worries; likewise.

fj-mkdeb.py is broken in 0.9.70 by this PR -
$ ./contrib/fj-mkdeb.py --disable-firetunnel --enable-apparmor
make: *** No rule to make target 'distclean'.  Stop.
That error can be worked around by running ./configure first, but then it
produces a deb package named
"firejail_0.9.70--disable-firetunnel_1_amd64.deb" with firetunnel support
enabled?

That is because as of this PR (#5154), the first argument to mkdeb.sh (and thus
also to fj-mkdeb.py) is the EXTRA_VERSION variable. So unless you want to
set EXTRA_VERSION, you have to pass an empty string as the first argument:

./contrib/fj-mkdeb.py '' --disable-firetunnel --enable-apparmor

And as of #5654, that is no longer the case; it should be possible to just pass
all arguments to mkdeb.sh (and to fj-mkdeb.py) directly:

./contrib/fj-mkdeb.py --disable-firetunnel --enable-apparmor

Also it is straightforward enough that fj-mkdeb.py now seems superfluous.

For the most part, yes; fj-mkdeb.py seems to boil down to the following:

#!/bin/sh

set -e

make distclean
./configure "$@" --prefix=/usr
make dist
./mkdeb.sh "$@"

Should fj-mkdeb.py just be removed?

Possibly; not sure if anyone else is using it. I might open a PR to remove it
(or maybe turn it into the above script) after #5654.

The "deb" target depends on the "dist" target, which creates an archive from DISTFILES. The arguments to ./configure are misleading, as they do not affect the archive that is used by `make deb`. That is the case because the configure output files (config.mk and config.sh) are not copied into the dist archive, only their input files (config.mk.in and config.sh.in). In order to affect the .deb package, the configure arguments have to be passed to mkdeb.sh, which then forwards them to ./configure itself. Note: This does not apply to the rpm-based jobs, as `make rpms` uses the files directly rather than using the dist archive. Relates to netblue30#5154.

The official .deb package is always built with apparmor support, so use `--enable-apparmor` in mkdeb.sh and remove the "deb-apparmor" target in order to reduce redundancy. Note that custom configure options may be specified by calling ./mkdeb.sh directly. For example, to build the .deb package without apparmor support, instead of running `make deb`, the following commands can be used: make dist ./mkdeb.sh --disable-apparmor Also, change the `build_apparmor` GitLab CI job into `build_no_apparmor`, which is intended to check that building without apparmor still works. Note: This commit makes the resulting .deb package not have an "-apparmor" suffix (see `EXTRA_VERSION` in mkdeb.sh), to avoid redundancy (as having apparmor support becomes the default). Misc: This is a follow-up to netblue30#5654. Relates to netblue30#5154 netblue30#5176 netblue30#5547.

For consistency with mkdeb.sh. Note: The default arguments and support for argument overriding was added to to mkrpm.sh on commit 3d97332 ("Add configure options when building rpm (netblue30#3422)", 2020-05-19). The support for appending arguments was added to mkdeb.sh on commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR netblue30#5154.

kmk3 requested review from reinerh, topimiettinen and rusty-snake May 20, 2022 20:08

kmk3 added this to In progress in Release 0.9.70 via automation May 20, 2022

reinerh reviewed May 21, 2022

View reviewed changes

kmk3 marked this pull request as draft May 21, 2022 16:36

kmk3 mentioned this pull request May 26, 2022

build: deduplicate configure-time vars into new config files #5140

Merged

kmk3 force-pushed the build-clean-up-dist branch from ade13a1 to c59ab8a Compare May 27, 2022 21:01

kmk3 changed the title ~~build: stop overriding user config on mkdeb.sh & remove fj-mkdeb.py~~ mkdeb.sh.in: pass remaining arguments to ./configure May 27, 2022

kmk3 force-pushed the build-clean-up-dist branch from c59ab8a to a6a94ea Compare May 27, 2022 21:13

kmk3 added 2 commits May 27, 2022 18:21

mkdeb.sh.in: remove unused PACKAGE_TARNAME/TOP vars

5247a6c

PACKAGE_TARNAME was added on commit 87e7b31 ("Configure Debian package with AA and SELinux options", 2020-05-13) / PR netblue30#3414. TOP was added on commit ed4a24c ("porting make deb-apparmor from LTS build", 2019-01-26).

kmk3 force-pushed the build-clean-up-dist branch from a6a94ea to a292ecf Compare May 27, 2022 21:21

kmk3 marked this pull request as ready for review May 27, 2022 21:45

reinerh reviewed May 28, 2022

View reviewed changes

Makefile.in Show resolved Hide resolved

reinerh approved these changes May 28, 2022

View reviewed changes

kmk3 force-pushed the build-clean-up-dist branch from a292ecf to 9a0fbbd Compare May 29, 2022 22:02

netblue30 merged commit 7ab6c49 into netblue30:master May 30, 2022

kmk3 deleted the build-clean-up-dist branch May 30, 2022 16:59

kmk3 mentioned this pull request Jun 5, 2022

mkdeb.sh.in: stop enabling apparmor #5176

Merged

kmk3 mentioned this pull request Jun 8, 2022

Revert "Makefile.in: stop running distclean on dist" #5182

Merged

kmk3 added a commit that referenced this pull request Jun 8, 2022

RELNOTES: add build entries

c20019a

Relates to #5133 #5154.

kmk3 moved this from In progress to Done (on RELNOTES) in Release 0.9.70 Jun 8, 2022

kmk3 added a commit that referenced this pull request Jun 18, 2022

update_deb.sh: fix not building with apparmor support

fddab1d

This amends commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to ./configure", 2022-05-13) / PR #5154. See also #5176.

kmk3 mentioned this pull request Feb 10, 2023

build: mkdeb.sh: pass all arguments to ./configure #5654

Merged

kmk3 mentioned this pull request Feb 17, 2023

build: deb: enable apparmor by default & remove deb-apparmor #5668

Merged

kmk3 mentioned this pull request Jul 27, 2023

build: dist and asc improvements #5916

Merged

kmk3 mentioned this pull request Dec 13, 2023

build: mkrpm.sh: append instead of override configure args #6126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mkdeb.sh.in: pass remaining arguments to ./configure #5154

mkdeb.sh.in: pass remaining arguments to ./configure #5154

kmk3 commented May 20, 2022 •

edited

kmk3 commented May 20, 2022

reinerh May 21, 2022

kmk3 May 21, 2022

kmk3 May 27, 2022

kmk3 Jun 8, 2022

laniakea64 commented May 24, 2022

lgtm-com bot commented May 27, 2022

kmk3 commented May 27, 2022

laniakea64 commented Jun 9, 2022

kmk3 commented Feb 10, 2023

mkdeb.sh.in: pass remaining arguments to ./configure #5154

mkdeb.sh.in: pass remaining arguments to ./configure #5154

Conversation

kmk3 commented May 20, 2022 • edited

kmk3 commented May 20, 2022

reinerh May 21, 2022

Choose a reason for hiding this comment

kmk3 May 21, 2022

Choose a reason for hiding this comment

kmk3 May 27, 2022

Choose a reason for hiding this comment

kmk3 Jun 8, 2022

Choose a reason for hiding this comment

laniakea64 commented May 24, 2022

lgtm-com bot commented May 27, 2022

kmk3 commented May 27, 2022

laniakea64 commented Jun 9, 2022

kmk3 commented Feb 10, 2023

kmk3 commented May 20, 2022 •

edited