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

Makefile: don't reference deleted INSTALL file in apidoc target #117

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

olifre
Copy link
Contributor

@olifre olifre commented Jun 23, 2023

Fixes build error if all targets are made.

Fixes build error if all targets are made.
@giacomini
Copy link
Member

Thanks for the PR. However I don't see any build error related to the INSTALL file. How can I reproduce it?

@giacomini giacomini self-assigned this Jul 2, 2023
@olifre
Copy link
Contributor Author

olifre commented Jul 2, 2023

I have stumbled upon this by using a regular packaging tool (in this case, Gentoo's Portage). Packaging tools in general avoid calling autoreconf but call the actual steps individually, which increases reproducibility and highlights leftovers in automake configurations better.

I checked which commands Portage calls in this case, and it burns down to:

mkdir -p aux src/autogen
libtoolize --install --copy --force --automake
aclocal -I m4 
autoconf --force
autoheader
automake --add-missing --copy --foreign --force-missing
./configure
make -j10

It seems autoreconf -i -f has some tricks to work around the issue, it seems to find INSTALL referenced in the target and decides to generate it with some dummy content. That's probably why it's not seen in the CI here.

@olifre
Copy link
Contributor Author

olifre commented Jul 2, 2023

Note that another option would be to add:

AUTOMAKE_OPTIONS = gnu

to Makefile.am. This enforces generation of all missing files required by the gnu strictness level of automake during the automake --add-missing step, so the INSTALL file with the usual dummy content will be generated even without autoreconf -i.

@giacomini
Copy link
Member

I'm reluctant to change the build options. I don't want to risk that what comes out doesn't satisfy policies for other platforms. Would it be ok with you if I commit the INSTALL file, as it was already the case some time ago?

@msalle
Copy link
Contributor

msalle commented Jul 4, 2023

I'm reluctant to change the build options. I don't want to risk that what comes out doesn't satisfy policies for other platforms. Would it be ok with you if I commit the INSTALL file, as it was already the case some time ago?

I think it's actually better to have an INSTALL file committed, even if it's the default.

@olifre
Copy link
Contributor Author

olifre commented Jul 4, 2023

Would it be ok with you if I commit the INSTALL file, as it was already the case some time ago?

Sure, that would of course also be fine (note the alternative would be to drop the reference to the file as done in this PR, i.e. not install the file).

In general, since the gnu style basically means an INSTALL file should be present and autoreconf -i just creates it, either having it committed (and changing nothing else) or dropping the reference to it from the makefile (as done in this PR) seems like the two least dangerous choices.

@ellert
Copy link
Contributor

ellert commented Sep 26, 2023

The problem is the --foreign option to the call to automake in your build script. This overrides the default which is --gnu.
Without this the automake call will create the INSTALL file for you.

@olifre
Copy link
Contributor Author

olifre commented Sep 27, 2023

The problem is the --foreign option to the call to automake in your build script. This overrides the default which is --gnu.

The option --foreign is added by the package management infrastructure on Gentoo Linux if files mandated by GNU strictness are missing:
https://gitweb.gentoo.org/repo/gentoo.git/tree/eclass/autotools.eclass?id=0e74da3a3d3fd1e90fb779e7ce3a1147ce3d5300#n455
I could of course carry a downstream patch creating the INSTALL file or explicitly add AUTOMAKE_OPTIONS = gnu to Makefile.am via a downstream patch, but it would be nicer to have this upstream.

@ellert
Copy link
Contributor

ellert commented Sep 27, 2023

Installing the INSTALL file doesn't really make sense anyway. So accepting this PR would make sense anyway I think.

@giacomini giacomini merged commit 410be29 into italiangrid:develop Apr 29, 2024
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.

4 participants