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

distribute configure, aclocal.m4 #1492

Closed
wants to merge 1 commit into from

Conversation

mezzarobba
Copy link
Contributor

No description provided.

@mezzarobba mezzarobba marked this pull request as draft October 11, 2023 07:51
@mezzarobba mezzarobba marked this pull request as ready for review October 11, 2023 08:13
@edgarcosta
Copy link
Member

fyi, git archive supports --format=tar.gz, thus one can simplify the first line of dist

@mezzarobba
Copy link
Contributor Author

That's also what I thought, but I didn't bother to check...

@mezzarobba
Copy link
Contributor Author

Actually we could even do something like

diff --git a/Makefile.in b/Makefile.in
index 146c6dcce..d107957f5 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -790,11 +790,12 @@ endif
 configure aclocal.m4 src/config.h.in &: bootstrap.sh configure.ac acinclude.m4
        ./$<

-DIST_PREFIX := flint-$(FLINT_MAJOR).$(FLINT_MINOR).$(FLINT_PATCH)/
+DIST_NAME := flint-$(FLINT_MAJOR).$(FLINT_MINOR).$(FLINT_PATCH)

-dist: configure aclocal.m4 src/config.h.in
-       git archive --format tar.gz --prefix ${DIST_PREFIX}src/ $(patsubst %,--add-file %,$(filter src/%,$^)) --prefix ${DIST_PREFIX}  $(patsubst %,--add-file %,$(filter-out src/%,$^)) origin/flint-$(FLINT_MAJOR).$(FLINT_MINOR) > ../flint-$(FLINT_MAJOR).$(FLINT_MINOR).$(FLINT_PATCH).tar.gz
-       git archive --format zip --prefix ${DIST_PREFIX}src/ $(patsubst %,--add-file %,$(filter src/%,$^)) --prefix ${DIST_PREFIX}  $(patsubst %,--add-file %,$(filter-out src/%,$^)) origin/flint-$(FLINT_MAJOR).$(FLINT_MINOR) > ../flint-$(FLINT_MAJOR).$(FLINT_MINOR).$(FLINT_PATCH).zip
+../$(DIST_NAME).%: FORCE
+       git archive --format $* --prefix ${DIST_NAME}/src/ --add-file src/config.h.in --prefix ${DIST_NAME}/ --add-file configure --add-file aclocal.m4 origin/flint-$(FLINT_MAJOR).$(FLINT_MINOR) > $@
+
+dist: ../$(DIST_NAME).tar.gz ../$(DIST_NAME).zip

 ################################################################################
 # debugging
@@ -803,4 +804,6 @@ dist: configure aclocal.m4 src/config.h.in
 print-%:
        @echo "$*=$($*)"

+FORCE:
+
 .PHONY: all library shared static examples profile tests check tune valgrind clean distclean install uninstall dist %_TEST_RUN %_VALGRIND_RUN print-% coverage

Not sure if this is better of worse. Any opinions?

@edgarcosta
Copy link
Member

It is better, as I only need to parse one line instead of two identical lines.

What is the point of FORCE?

@mezzarobba
Copy link
Contributor Author

Since the output is based on the contents of the git repo itself and not just on the worktree, I think we want make dist to recreate the archives even if they appear to be up-to-date. Since the rule contains a wildcard, we cannot just declare it as .PHONY.

@mezzarobba mezzarobba marked this pull request as draft October 11, 2023 09:53
@mezzarobba

This comment was marked as outdated.

@mezzarobba mezzarobba marked this pull request as ready for review October 11, 2023 10:03
@mezzarobba
Copy link
Contributor Author

ok, here is a version that works for me...

@edgarcosta
Copy link
Member

Thanks for the explanation

@mezzarobba mezzarobba added this to the flint-3.0 milestone Oct 11, 2023
@AndreasEnge
Copy link
Contributor

I tried it out, and the code creates a .tar.gz that I can unpack and that allows me to do "./configure" and "make".

As someone used to autotools, the different behaviour is a bit surprising; I am used to trying "make dist" (actually, "make distcheck") just in my source code checkout, whereas here a branch "origin/flint-x.y" is expected. For the maintainer, I suppose this patch does the desired thing.

@fingolfin
Copy link
Collaborator

This seems to duplicate the work I already did on #1449 ?

@mezzarobba
Copy link
Contributor Author

Indeed! Sorry for the noise.

@mezzarobba mezzarobba closed this Oct 11, 2023
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 4, 2023
    
Upgrade to flint3.

Current Sage versions are not compatible with flint ≥ 3, and, though the
diff is not huge, there are enough changes that versions including this
PR will be incompatible with flint < 3.

Fixes sagemath#20003.
Closes sagemath#35993 as no longer relevant.

Related PRs in upstream projects:
* Singular/Singular#1177
* flintlib/flint#1408
* flintlib/flint#1489
* flintlib/flint#1492
* flintlib/flint#1611
* algebraic-solving/msolve#76
* flatsurf/e-antic#264

Additional changes still needed for optional packages to work:
* sagemath#36677
* upgrade e-antic
* possibly more

Planned follow-ups:
* sagemath#36449
* sagemath#36433
    
URL: sagemath#35848
Reported by: Marc Mezzarobba
Reviewer(s): Vincent Delecroix
vbraun pushed a commit to vbraun/sage that referenced this pull request Dec 5, 2023
    
Upgrade to flint3.

Current Sage versions are not compatible with flint ≥ 3, and, though the
diff is not huge, there are enough changes that versions including this
PR will be incompatible with flint < 3.

Fixes sagemath#20003.
Closes sagemath#35993 as no longer relevant.

Related PRs in upstream projects:
* Singular/Singular#1177
* flintlib/flint#1408
* flintlib/flint#1489
* flintlib/flint#1492
* flintlib/flint#1611
* algebraic-solving/msolve#76
* flatsurf/e-antic#264

Additional changes still needed for optional packages to work:
* sagemath#36677
* upgrade e-antic
* possibly more

Planned follow-ups:
* sagemath#36449
* sagemath#36433
    
URL: sagemath#35848
Reported by: Marc Mezzarobba
Reviewer(s): Vincent Delecroix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants