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.

Sign up for GitHub

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

Jump to bottom

CI: add autotools, out-of-tree, debug build to distro check job #12088

Closed
wants to merge 11 commits into from
Closed

CI: add autotools, out-of-tree, debug build to distro check job #12088

wants to merge 11 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 11, 2023 •
edited

Add a job that builds curl from a generated source tarball sample, with
autotools, out-of-tree, in debug mode.

Ref: #12085
Closes #12088

@vszakats vszakats marked this pull request as draft October 11, 2023 09:09
@github-actions github-actions bot added the CI Continuous Integration label Oct 11, 2023
@vszakats vszakats mentioned this pull request Oct 11, 2023
Closed
@vszakats
Copy link
Member Author 

make[3]: *** No rule to make target 'Makefile.dist', needed by 'distdir-am'.  Stop.

https://github.com/curl/curl/actions/runs/6480792976/job/17597019192#step:7:3863

@vszakats
Copy link
Member Author

Stuck at:

make[3]: *** No rule to make target 'Makefile.dist', needed by 'distdir-am'.  Stop.

https://github.com/curl/curl/actions/runs/6480934827/job/17597469241#step:7:2658

@vszakats vszakats changed the title TESTING: ci test distcheck ci: try adding make distcheck Oct 11, 2023
@vszakats
Copy link
Member Author

vszakats commented Oct 11, 2023 •
edited

This was "easy": Makefile.dist is referenced from Makefile.am in EXTRA_DIST, while apparently it shouldn't be there. First because distcheck breaks on this, second because this file isn't part of the of the source tarball (not with this filename anyway).

Next problem: ERROR: files left in build directory after distclean: followed by a list of files created during the build process:
https://github.com/curl/curl/actions/runs/6483154400/job/17604109905#step:7:4028

Next-next problem: distcheck actually doesn't complain about a missing src/.checksrc at all. Perhaps this isn't even what it's meant to do, though I swear to have been warned about missing files in e.g. libssh2, when running distcheck.

Running it with --enable-debug makes it fail without that particular file, but distcheck isn't needed for that, and autotools debug version of verify-out-of-tree-cmake would do the job (not tested yet).

@vszakats
Copy link
Member Author

Dropped the make distcheck idea, though in theory it's probably the way to fix this class of problem.
Added a debug out-of-tree configure job instead.

Example of it catching the missing src/.checksrc:
https://github.com/curl/curl/actions/runs/6483454571/job/17605256636#step:3:4907

@vszakats vszakats changed the title ci: try adding make distcheck ci: add autotools + out-of-tree + debug build to distro check job Oct 11, 2023
@vszakats vszakats marked this pull request as ready for review October 11, 2023 14:16
@vszakats
Copy link
Member Author

vszakats commented Oct 11, 2023 •
edited

Now it's this:

/usr/bin/install: cannot create regular file '/usr/local/lib/libcurl.so.4.8.0': Permission denied

I'm giving up!

@vszakats
Copy link
Member Author

Okay, let's try without install first...

@bagder
Copy link
Member

bagder commented Oct 11, 2023 •
edited

because distcheck breaks on this, second because this file isn't part of the of the source tarball (not with this filename anyway).

make dist makes all *.dist files get included in the tarball without the .dist extension. So the Makefile.dist file ends up as just Makefile in the tarball.

@vszakats
Copy link
Member Author

Phew, green now:
https://github.com/curl/curl/actions/runs/6483854257/job/17606626203?pr=12088

This is already rebased on master with the src/.checksrc fix.

@vszakats
Copy link
Member Author

@bagder: Do you think we should delete Makefile.dist off the EXTRA_DIST list, or should is stay there? (this patch deletes it at the moment)

@dfandrich
Copy link
Contributor

dfandrich commented Oct 11, 2023 via email

@vszakats
Copy link
Member Author

That did the trick, thanks Dan!

@bagder
Copy link
Member

bagder commented Oct 12, 2023

@bagder: Do you think we should delete Makefile.dist off the EXTRA_DIST list, or should is stay there? (this patch deletes it at the moment)

I believe the reasoning for including Makefile.dist in the tarball is that it lets a user make a subsequent release with maketgz based on only the files provided in the tarball, and I believe that is a sensible policy. This, since the root Makefile might get overwritten if for example configure is invoked from within the source directory.

So yes: I think it can remain in EXTRA_DIST.

@vszakats
Copy link
Member Author

vszakats commented Oct 12, 2023 •
edited

@bagder: Thanks, I'll revert this, but Makefile.dist is not present in the release tarball (I'm guessing due to the error showcased above), which might need another fix.

@bagder
Copy link
Member

bagder commented Oct 12, 2023

@bagder: Thanks, I'll revert this, but Makefile.dist is not present in the release tarball (I'm guessing due to the error showcased above), which might need another fix.

Ah yes, and that's a slightly tricky one since we have that hook that puts .dist files into the tarball without that extension:

curl/Makefile.am

Lines 171 to 178 in a20d7bd

dist-hook:
rm -rf $(top_builddir)/tests/log
find $(distdir) -name "*.dist" -exec rm {} \;
(distit=`find $(srcdir) -name "*.dist" | grep -v ./ares/`; \
for file in $$distit; do \
strip=`echo $$file | sed -e s/^$(srcdir)// -e s/\.dist//`; \
cp -p $$file $(distdir)$$strip; \
done)

@vszakats vszakats changed the title ci: add autotools + out-of-tree + debug build to distro check job CI: add autotools, out-of-tree, debug build to distro check job Oct 12, 2023
@vszakats vszakats closed this in 2e283c6 Oct 12, 2023
@vszakats vszakats deleted the ci-test-distcheck branch October 12, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mback2k mback2k Awaiting requested review from mback2k

@bagder bagder Awaiting requested review from bagder

Assignees
No one assigned
Labels
CI Continuous Integration tests
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vszakats @bagder @dfandrich