Move checks and package initialization after build #2646

ffesti · 2023-09-05T11:00:04Z

as far as possible. This allows setting these tags during the build using the dynamic spec feature.

Move

copyTagsDuringParse
requiredTags
optionalTags
isMemberInEntry()
checkForValidArchitectures()
checkForRequired()
checkForDuplicates()
fillOutMainPackage()
copyInheritedTags()

from build/parsePreamble.c to build/parseSpec.c

Use an already existing main package in parsePreamble()

New function finalizeSourceHeader() to move some initialization after build.

Add RPMSPEC_DONTFINALIZE flag to delay package finialization after the build in rpmbuild. rpmspec and API parsing without the flag still finializes the packages right away.

pmatilai

Eek. I can see why you want to do something like this, but this kind of mega-patch is hard to review and totally unbisectable.

Split it into more commits. For things that just move but don't actually change, temporarily change them to non-static with prototypes in rpmbuild_internal.h. Then do the actual change(s), which become actually revievable now, and finally do the actual move of the those unchanged helpers in a commit that only moves and doesn't change.

A change this drastic is almost certainly going to scare a whole flock of critters out of their dark corners. There are endless subtle corner-cases in the package initialization stuff, that s*** depends on, that aren't covered by the test-suite.

ffesti · 2023-09-14T07:33:11Z

This is the major part of what is needed for #1240

pmatilai · 2023-09-26T10:16:18Z

The actual commits look a whole lot more approachable now, only the GH interface is totally inadequate for this kind of job... but lets try.

pmatilai · 2023-09-26T10:19:27Z

Actually, please drop the "move functions" commit out of this set. That's what makes so unrevieable on GH, yet that's not even an interesting commit in itself 😅 We can always move it later if deemed important.

pmatilai · 2023-09-26T10:30:49Z

In the meanwhile, spotted at least one problem: dropping the NVR argument from checkForRequired() breaks in the case of Name tag missing from the main package. As that can only happen with the main package, should be easy enough to work around though.

pmatilai · 2023-09-26T10:39:41Z

I'm getting a bunch of warnings about free() of uninitialized value in finalizeSpec() and the warnings are valid, as the first goto can jump over the declaration entirely.

But, that should be tripping up the CI compile stage already. Have we lost ENABLE_WERROR=ON there? (not the fault of this patch of course)

pmatilai · 2023-09-26T11:50:02Z

include/rpm/rpmspec.h

@@ -37,6 +37,7 @@ enum rpmSpecFlags_e {
    RPMSPEC_FORCE	= (1 << 1),
    RPMSPEC_NOLANG	= (1 << 2),
    RPMSPEC_NOUTF8	= (1 << 3),
+    RPMSPEC_DONTFINALIZE = (1 << 4),


Use "NO" instead of "DONT" for consistency with the rest of rpm.

pmatilai · 2023-09-26T12:08:34Z

What exactly is this supposed to mean in the context of the "move checks and package init after build" commit?

NAME, VERSION, RELEASE, (EPOCH) is needed for all sub packages and the source rpm for the build. The srpm also needs ARCH, OS and the BuildRequires.

Just tested, and rpmbuild will now merrily try to build something when Name, Version and Release aren't specified in the spec and fail in myriad of ways because so many things inside the build depend on them. Dynamically generated stuff may be fun but that is too far.

ffesti · 2023-10-03T07:33:23Z

OK, I addressed the comments above:

Re-Added a check for the NVR tags, renamed RPMSPEC_DONTFINALIZE, added a test case and fixed issues that the test case turned up.

pmatilai · 2023-10-04T09:21:05Z

Seeing requiredTagsForBuild inspired some thoughts for the basical reverse cases of things that cannot be handled from generated content:

What happens if somebody generates a BuildArch line from inside the build? Other than noarch sub-packages that is.

What happens with stuff like BuildRequires / %generate_buildrequires? Those would end up in the src.rpm requires and the parsed spec in the src.rpm, but since they're not in the spec they wouldn't be enforced in the next build either, I guess? Or %prep/%build/%install?

And that must be just scratching the surface of such issues...

ffesti · 2023-10-04T10:33:12Z

Yes, this thought has occurred to me, too. I have not addressed this here as it is mainly an issue of the original dynamic spec change. But it is something we need to address.

Funny enough we could actually allow %prep to create later build scripts. Ofc this doesn't work right now. Also there isn't really a point.

pmatilai · 2023-10-10T11:21:17Z

BTW one cosmetic issue here in all but the first commits: the commit summary line and actual message should be independent of each other. Continuing the description where the summary line left off seems to be a bit of a habbit of yours (ie not just in this PR), please don't do that.

pmatilai · 2023-10-10T11:30:39Z

include/rpm/rpmspec.h

@@ -37,6 +37,7 @@ enum rpmSpecFlags_e {
    RPMSPEC_FORCE	= (1 << 1),
    RPMSPEC_NOLANG	= (1 << 2),
    RPMSPEC_NOUTF8	= (1 << 3),
+    RPMSPEC_NOFINALIZATION  = (1 << 4),


I actually meant just "NOFINALIZE" which isn't quite as annoyingly long 😅

pmatilai · 2023-10-10T11:36:29Z

Have you tested this with some of the more complicated specs in Fedora, eg kernel / texlive / the Lua-generated stuff and so on? Our test-suite doesn't really exercise the spec parsing voodoo that deeply, and a change this big in that department makes this sheep rather nervous.

pmatilai · 2023-10-10T11:56:33Z

build/parseSpec.c

+
+    char *platform = rpmExpand("%{_target_platform}", NULL);
+    char *os = rpmExpand("%{_target_os}", NULL);
+    char *optflags = rpmExpand("%{optflags}", NULL);


These are still causing the warnings I mentioned earlier:

/home/pmatilai/repos/rpm/build/parseSpec.c: In function ‘finalizeSpec’: /home/pmatilai/repos/rpm/build/parseSpec.c:1160:5: warning: ‘platform’ may be used uninitialized [-Wmaybe-uninitialized] 1160 | free(platform); | ^~~~~~~~~~~~~~ /home/pmatilai/repos/rpm/build/parseSpec.c:1121:11: note: ‘platform’ was declared here 1121 | char *platform = rpmExpand("%{_target_platform}", NULL); | ^~~~~~~~ /home/pmatilai/repos/rpm/build/parseSpec.c:1161:5: warning: ‘os’ may be used uninitialized [-Wmaybe-uninitialized] 1161 | free(os); | ^~~~~~~~ /home/pmatilai/repos/rpm/build/parseSpec.c:1122:11: note: ‘os’ was declared here 1122 | char *os = rpmExpand("%{_target_os}", NULL); | ^~ /home/pmatilai/repos/rpm/build/parseSpec.c:1162:5: warning: ‘optflags’ may be used uninitialized [-Wmaybe-uninitialized] 1162 | free(optflags); | ^~~~~~~~~~~~~~ /home/pmatilai/repos/rpm/build/parseSpec.c:1123:11: note: ‘optflags’ was declared here 1123 | char *optflags = rpmExpand("%{optflags}", NULL); | ^~~~~~~~

The reason these aren't showing up in CI is that CI is built without any optimizations.
Need to tweak that a bit...

Okay, rebase to get the warnings -> compile fail on CI.

pmatilai · 2023-10-11T07:18:06Z

tests/data/SPECS/dynamic.spec

@@ -30,6 +39,7 @@ echo "Test for dynamically generated spec files" >> %{specpartsdir}/docs.specpar
 echo "%files docs" >> $RPM_SPECPARTS_DIR/docs.specpart
 echo "%doc	FAQ" >> $RPM_SPECPARTS_DIR/docs.specpart

+


Unrelated whitespace

pmatilai · 2023-10-11T07:35:38Z

tests/data/SPECS/dynamic.spec

+echo "Distribution: RPM test suite." >> %{specpartsdir}/docs.specpart
+echo "URL: http://rpm.org" >> %{specpartsdir}/docs.specpart
+echo "Summary: dynamic hello -- hello, world rpm" >> %{specpartsdir}/docs.specpart
+}


I'd put these to separate file, just to get more coverage.

Also, do add a separate test for a syntax error while parsing dynamically generated stuff, which is a different case from checking for required tags (for which there is a test). The error message in the syntax case is pretty terrible because there's no file information attached to the message (although there are hints in the output log), but AFAICS that doesn't change here so I'll just file a separate ticket on that.

This will become handy in the following changes. It also changes the error message to only show the name of the sub package instead of the NVR. But as package names need to be unique this should not be an issue. It also removes version and release as a dependency of the error message.

as far as possible NAME, VERSION, RELEASE, (EPOCH) are still needed for all sub packages and the source rpm for the build. Checks for those need to stay in place. The srpm also needs ARCH, OS and the BuildRequires. So those still get set early. Add RPMSPEC_NOFINALIZATION flag to delay the package initialation after the parsing of the dynamic spec parts for normal builds while doing it right away when just parsing the spec otherwise (e.g. with rpmspec).

as we now do that at the end of the parse run Still check for the tags needed for build: Name, Version, Release

even on secondary parse runs. This allows declaring tags not essential for building in dynamic spec parts.

setting properties of the main package

This tests wrong/unknown tags and a doublicated Summary tag

ffesti · 2023-10-11T15:25:59Z

OK, everything that didn't get moved to its own ticket should be addressed. Renamed the constant to NOFINALIZE, fixed warnings and added to more test cases with parsing errors.

I am a bit confused that giving Summary two times is only a warning and doesn't break the build. BUt that's not something the patch should have changed.

pmatilai

Yeah I was a bit surprised too a duplicate tag is only a warning. It's probably one of the many cases where people have been abusing the behavior for something so long that we first introduced it as a warning with the intention of changing it to an error later. Probably many many years ago 😅

Anyway, I think this is good to go now. We wont be discovering potential new breakage by having it sit in a PR either.

pmatilai requested changes Sep 5, 2023

View reviewed changes

ffesti force-pushed the dynamic branch 2 times, most recently from 6b381f1 to 82cb2da Compare September 19, 2023 10:16

pmatilai reviewed Sep 26, 2023

View reviewed changes

ffesti force-pushed the dynamic branch from 82cb2da to ab41937 Compare October 3, 2023 07:29

ffesti mentioned this pull request Oct 4, 2023

Prevent Dynamic Spec part to create things needed earlier #2693

Closed

pmatilai reviewed Oct 10, 2023

View reviewed changes

pmatilai reviewed Oct 11, 2023

View reviewed changes

ffesti added 2 commits October 11, 2023 16:44

Make functions available to be moved later on

6bb9f49

ffesti force-pushed the dynamic branch from ab41937 to 10eb812 Compare October 11, 2023 14:47

ffesti added 5 commits October 11, 2023 17:19

Remove checks during parsing of packages

80feaf6

as we now do that at the end of the parse run Still check for the tags needed for build: Name, Version, Release

Always start parsing in the preable of the main package

2581fec

even on secondary parse runs. This allows declaring tags not essential for building in dynamic spec parts.

Add test case for dynamic spec

8c02174

setting properties of the main package

Add more dynamic spec test cases

6b876e0

This tests wrong/unknown tags and a doublicated Summary tag

ffesti force-pushed the dynamic branch from 10eb812 to 6b876e0 Compare October 11, 2023 15:19

pmatilai approved these changes Oct 12, 2023

View reviewed changes

pmatilai merged commit d13cea3 into rpm-software-management:master Oct 12, 2023
1 check passed

dmnks added the packaging Package building, SPEC files, etc. label Nov 28, 2023

ffesti deleted the dynamic branch February 16, 2024 10:47

dmnks added the cleanup label Mar 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move checks and package initialization after build #2646

Move checks and package initialization after build #2646

ffesti commented Sep 5, 2023

pmatilai left a comment •

edited

Loading

ffesti commented Sep 14, 2023

pmatilai commented Sep 26, 2023

pmatilai commented Sep 26, 2023 •

edited

Loading

pmatilai commented Sep 26, 2023

pmatilai commented Sep 26, 2023 •

edited

Loading

pmatilai Sep 26, 2023

pmatilai commented Sep 26, 2023

ffesti commented Oct 3, 2023

pmatilai commented Oct 4, 2023 •

edited

Loading

ffesti commented Oct 4, 2023

pmatilai commented Oct 10, 2023

pmatilai Oct 10, 2023

pmatilai commented Oct 10, 2023

pmatilai Oct 10, 2023 •

edited

Loading

pmatilai Oct 10, 2023

pmatilai Oct 10, 2023

pmatilai Oct 11, 2023

pmatilai Oct 11, 2023

ffesti commented Oct 11, 2023

pmatilai left a comment

		@@ -30,6 +39,7 @@ echo "Test for dynamically generated spec files" >> %{specpartsdir}/docs.specpar
		echo "%files docs" >> $RPM_SPECPARTS_DIR/docs.specpart
		echo "%doc FAQ" >> $RPM_SPECPARTS_DIR/docs.specpart

Move checks and package initialization after build #2646

Move checks and package initialization after build #2646

Conversation

ffesti commented Sep 5, 2023

pmatilai left a comment • edited Loading

Choose a reason for hiding this comment

ffesti commented Sep 14, 2023

pmatilai commented Sep 26, 2023

pmatilai commented Sep 26, 2023 • edited Loading

pmatilai commented Sep 26, 2023

pmatilai commented Sep 26, 2023 • edited Loading

pmatilai Sep 26, 2023

Choose a reason for hiding this comment

pmatilai commented Sep 26, 2023

ffesti commented Oct 3, 2023

pmatilai commented Oct 4, 2023 • edited Loading

ffesti commented Oct 4, 2023

pmatilai commented Oct 10, 2023

pmatilai Oct 10, 2023

Choose a reason for hiding this comment

pmatilai commented Oct 10, 2023

pmatilai Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

pmatilai Oct 10, 2023

Choose a reason for hiding this comment

pmatilai Oct 10, 2023

Choose a reason for hiding this comment

pmatilai Oct 11, 2023

Choose a reason for hiding this comment

pmatilai Oct 11, 2023

Choose a reason for hiding this comment

ffesti commented Oct 11, 2023

pmatilai left a comment

Choose a reason for hiding this comment

pmatilai left a comment •

edited

Loading

pmatilai commented Sep 26, 2023 •

edited

Loading

pmatilai commented Sep 26, 2023 •

edited

Loading

pmatilai commented Oct 4, 2023 •

edited

Loading

pmatilai Oct 10, 2023 •

edited

Loading