Skip to content

Conversation

@alejandro-colomar
Copy link
Collaborator

@alejandro-colomar alejandro-colomar commented Aug 2, 2025

'make distcheck' runs ./configure (among other things). That command should inherit the flags passed on the command line, as they are the flags necessary to build in the current system.

This also allows testing different configurations in the same system. Until now, we only tested the default automagic configuration. With this change, one can test the same default automagic configuration, by not passing any flags to ./configure, or they can test more specific configurations, by passing flags.

Cc: @lslebodn, @Karlson2k


Revisions:

v1b
  • Rebase
$ git rd 
1:  d036cfe4 = 1:  d28666c8 configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'
v1c
  • Rebase
$ git rd 
1:  d28666c8 = 1:  875dd4d5 configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'
v1d
  • Rebase
$ git rd 
1:  875dd4d5 = 1:  7326d227 configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'
v1e
  • Rebase
$ git rd 
1:  7326d227 = 1:  f88682fb configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'
v1f
  • Rebase
$ git rd 
1:  f88682fb0 = 1:  f5d5841c0 configure.ac, Makefile.am: Propagate ./configure flags to 'distcheck'

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Aug 2, 2025

Does this allow any further clanup? (Such as not needing to install dependencies on CI?)

@Karlson2k
Copy link
Contributor

Karlson2k commented Aug 2, 2025

There is a missing part.
You need to replace the AM_DISTCHECK_CONFIGURE_FLAGS assignment in the top-level Makefile.ac:

AM_DISTCHECK_CONFIGURE_FLAGS = @ac_configure_args@

Edit: No, you used AM_DISTCHECK_CONFIGURE_FLAGS directly.
Then you need to remove AM_DISTCHECK_CONFIGURE_FLAGS assignment from the top-level Makefile.ac

@Karlson2k
Copy link
Contributor

Does this allow any further clanup? (Such as not needing to install dependencies on CI?)

I'd do the opposite: remove "--disable-logind" from Ubuntu -based CI. Then it will be build-tested with logind integration, which is the most common scenario currently.

@alejandro-colomar alejandro-colomar marked this pull request as ready for review August 2, 2025 11:36
@Karlson2k
Copy link
Contributor

The code is valid.

However, for the records, I still think that automatic "configure" flags should be enough to build the package if minimal required dependencies are available.

@lslebodn
Copy link

lslebodn commented Aug 2, 2025

I still think that automatic "configure" flags should be enough to build the package
if minimal required dependencies are available.

I fully agree with 1st part, not about minimal required dependencies

This also allows testing different configurations in the same system. Until now, we only tested the default automagic configuration.

A nitpick; we do not need make distcheck for testing different configuration.
git clean -fdx && autoreconf -if && ./configure ${parametes here} && make check is enough.
and moreover testing would also depend on installed dependencies.

Anyway, make dist(check) is mostly for maintainer to ensure all necessary files will be in tarball before the release.
So users can run standard commands

  • tar xzf .tar.gz
  • cd
  • ./configure #parameters here
  • make # with BUILD/LINK flags
  • sudo make install

And because it is meant for maintainer you should ask other maintainers what kind of preferences they have.
I do not have any preference about this PR.

@Karlson2k
Copy link
Contributor

Anyway, make dist(check) is mostly for maintainer to ensure all necessary files will be in tarball before the release. So users can run standard commands

Right. make distcheck, make installcheck are for maintainers. To test for possible breakage.

  • tar xzf .tar.gz
  • cd
  • ./configure #parameters here
  • make # with BUILD/LINK flags
  • sudo make install

A small correction.
CC, CFLAGS, LDFLAGS etc. should be used with configure for better detection.
./configure --some-parsms CC=somecc CFLAGS="--fsomeflags" && make && sudo make install

@alejandro-colomar
Copy link
Collaborator Author

alejandro-colomar commented Aug 2, 2025 via email

@Karlson2k
Copy link
Contributor

Just as a reminder: the line AC_SUBST([AM_DISTCHECK_CONFIGURE_FLAGS], ["$ac_configure_args"]) will add AM_DISTCHECK_CONFIGURE_FLAGS to every Makefile generated by configure, while the top level Makefile is enough.

Alternatively, as I mentioned earlier, a purer solution would be lines in configure.ac:

AC_SUBST([ac_configure_args])
AM_SUBST_NOTMAKE([ac_configure_args])

and in the top Makefile:

AM_DISTCHECK_CONFIGURE_FLAGS = @ac_configure_args@

This will add a single line only to the top Makefile.

However, if a pure Makefiles are not a goal (which makes sense as they are clattered anyway), the current version will work fine.

@alejandro-colomar
Copy link
Collaborator Author

Just as a reminder: the line AC_SUBST([AM_DISTCHECK_CONFIGURE_FLAGS], ["$ac_configure_args"]) will add AM_DISTCHECK_CONFIGURE_FLAGS to every Makefile generated by configure, while the top level Makefile is enough.

Alternatively, as I mentioned earlier, a purer solution would be lines in configure.ac:

AC_SUBST([ac_configure_args])
AM_SUBST_NOTMAKE([ac_configure_args])

and in the top Makefile:

AM_DISTCHECK_CONFIGURE_FLAGS = @ac_configure_args@

This will add a single line only to the top Makefile.

However, if a pure Makefiles are not a goal (which makes sense as they are clattered anyway), the current version will work fine.

Since makefiles are generated, I don't mind how much crap they contain. I prefer a slim config file.

'make distcheck' runs ./configure (among other things).  That command
should inherit the flags passed on the command line, as they are the
flags necessary to build in the current system.

This also allows testing different configurations in the same system.
Until now, we only tested the default automagic configuration.  With
this change, one can test the same default automagic configuration, by
not passing any flags to ./configure, or they can test more specific
configurations, by passing flags.

This allows removing the hardcoded AM_DISTCHECK_CONFIGURE_FLAGS in the
"Makefile.am".

Signed-off-by: Alejandro Colomar <[email protected]>
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