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

BuildPackages.sh: "make clean" before full build #3480

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

fingolfin
Copy link
Member

This way, if a user re-runs BuildPackages.sh in order to re-build packages,
there is a higher chance that this works as expected, instead of mixing old
compilation results with new ones.

For GAP packages with non-autoconf based build systems, we re-run configure
after the make clean, as for many of them, the latter removes their
generated Makefile (in violation of well-established UNIX conventions).

This way, if a user re-runs BuildPackages.sh in order to re-build packages,
there is a higher chance that this works as expected, instead of mixing old
compilation results with new ones.

For GAP packages with non-autoconf based build systems, we re-run configure
after the `make clean`, as for many of them, the latter removes their
generated Makefile (in violation of well-established UNIX conventions).
@fingolfin fingolfin added topic: build system topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker) labels May 30, 2019
@ChrisJefferson
Copy link
Contributor

I seem to remember doing this in the past, and finding some packages, on make clean, remove their documentation, and then make not breaking it back.

@ChrisJefferson
Copy link
Contributor

I'm trying to find the package which did this, and I can't seem to find it. Maybe it was an earlier problem and has been fixed.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 85.168% when pulling 1ac9cfc on fingolfin:mh/clean-BuildPackages into 5d81b55 on gap-system:master.

@ChrisJefferson
Copy link
Contributor

If I comparing building the packages, to doing "build, clean, build" then the following files are missing (of note, there is the odd minor file here and there)

gbnp has some files in doc/examples and some test files

GeneralizedMorhismsForCAP, LinearAlgebraForCAP, ModulePresentationsForCAP all don't regenerate their html docs, and also remove a file called 'maketest.g'

@fingolfin
Copy link
Member Author

OK, then I'll bug the CAP folks (@sebasguts and @sebastianpos) about fixing their Makefiles then :-).

@fingolfin
Copy link
Member Author

@ChrisJefferson huh, but looking closer, I don't understand: those three CAP packages don't have a configure script, so this PR should not even affect them?!

@ChrisJefferson
Copy link
Contributor

I didn't notice you don't run 'make clean' on packages without a configure. It looks like all packages which feature problematic 'make clean' happen to not have a configure script either.

@fingolfin
Copy link
Member Author

So, I just did a fresh package bootstrap; then used cd pkg && git init . && git add . && git commit -m import to turn pkg into a git repository. Then I run this modified build script. Afterwards, git status reported this (plus of course a ton of untracked files created by the build process):

    deleted:    FPLSA-1.2.2/doc/manual.pdf
    deleted:    FPLSA-1.2.2/doc/manual.six
    deleted:    anupq-3.2.1/doc/ANUPQ.toc
    modified:   guava-3.14/src/leon/aclocal.m4
    modified:   guava-3.14/src/leon/configure
    modified:   guava-3.14/src/leon/install-sh
    modified:   guava-3.14/src/leon/missing

So, it seems that FPLSA is affected (and I'll make sure to fix that there), but nothing else, at least nothing in the distribution.

@fingolfin
Copy link
Member Author

I just released fplsa 1.2.3 to improve this.

Copy link
Member

@olexandr-konovalov olexandr-konovalov left a comment

Choose a reason for hiding this comment

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

I had the same concerns as in the discussion above - glad to find them answered.

@olexandr-konovalov
Copy link
Member

Should this be mentioned in release notes?

@fingolfin
Copy link
Member Author

Regarding mentioning this in the release notes:
pro: it's a change that could in theory affect user's, and so it might be helpful to mention it. Also, technically it's an "enhancement"...
con: it probably won't affect any users, and even if, I think the fraction of those who'd be then helped by mentioning this in the release notes is close to or equal zero

All in all, I am completely neutral on whether to mention it or not

@ChrisJefferson ChrisJefferson merged commit d72e6b3 into gap-system:master Jun 5, 2019
@fingolfin fingolfin deleted the mh/clean-BuildPackages branch June 5, 2019 09:28
@olexandr-konovalov
Copy link
Member

Also it may affect package authors... It will make no harm to mention it, so let's do that.

@olexandr-konovalov olexandr-konovalov added the release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes label Jun 12, 2019
@olexandr-konovalov olexandr-konovalov added this to the GAP 4.11.0 milestone Jun 12, 2019
@DominikBernhardt DominikBernhardt added release notes: added PRs introducing changes that have since been mentioned in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes labels Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: added PRs introducing changes that have since been mentioned in the release notes topic: build system topic: packages issues or PRs related to package handling, or specific to a package (for packages w/o issue tracker)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants