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

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

Merged
merged 3 commits into from
May 30, 2022

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented May 20, 2022

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
Copy link
Collaborator Author

kmk3 commented May 20, 2022

Depends on #5142.

Makefile.in Outdated
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 \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kmk3 kmk3 marked this pull request as draft May 21, 2022 16:36
@laniakea64
Copy link
Contributor

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?

@kmk3 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
@lgtm-com
Copy link

lgtm-com bot commented May 27, 2022

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

new alerts:

  • 1 for Unused import

kmk3 added 2 commits May 27, 2022 18:21
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
Copy link
Collaborator Author

kmk3 commented May 27, 2022

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

@kmk3 kmk3 marked this pull request as ready for review May 27, 2022 21:45
Makefile.in Show resolved Hide resolved
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.
@netblue30 netblue30 merged commit 7ab6c49 into netblue30:master May 30, 2022
@kmk3 kmk3 deleted the build-clean-up-dist branch May 30, 2022 16:59
kmk3 added a commit to kmk3/firejail that referenced this pull request Jun 5, 2022
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.
kmk3 added a commit to kmk3/firejail that referenced this pull request Jun 8, 2022
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.
kmk3 added a commit that referenced this pull request Jun 8, 2022
@laniakea64
Copy link
Contributor

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.

kmk3 added a commit that referenced this pull request Jun 11, 2022
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.
kmk3 added a commit that referenced this pull request Jun 18, 2022
This amends commit 9a0fbbd ("mkdeb.sh.in: pass remaining arguments to
./configure", 2022-05-13) / PR #5154.

See also #5176.
kmk3 added a commit to kmk3/firejail that referenced this pull request Feb 10, 2023
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
Copy link
Collaborator Author

kmk3 commented Feb 10, 2023

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

kmk3 added a commit to kmk3/firejail that referenced this pull request Feb 17, 2023
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.
kmk3 added a commit to kmk3/firejail that referenced this pull request Feb 17, 2023
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.
kmk3 added a commit to kmk3/firejail that referenced this pull request Dec 13, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

--enable-apparmor option is lost with make deb
4 participants