Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
Merged #2646 into master. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#event-10627010128 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai approved this pull request. 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 :sweat_smile: Anyway, I think this is good to go now. We wont be discovering potential new breakage by having it sit in a PR either. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1673067964 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1757953430 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@ffesti pushed 5 commits. 7258c44f688c7712af9cdcdb12227d3820e77879 Move checks and package initialization after build 80feaf69bf841293daa0707bccc546707ca7968e Remove checks during parsing of packages 2581fecd67178574f5e813e8c97fb5c21045d93f Always start parsing in the preable of the main package 8c02174effeb423da3059de422367f6af432927a Add test case for dynamic spec 6b876e04126d43a1a8ae3776d2a12d7a5d7f753f Add more dynamic spec test cases -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646/files/10eb812919f95f6c1a14d4261956d5b1217f279e..6b876e04126d43a1a8ae3776d2a12d7a5d7f753f You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@ffesti pushed 6 commits. a830cc6c8c009080a6d78b621c5206f2c4059bb2 Drop NVR parameter to make them easier to reuse 6bb9f49f4380c8ec96ea5ffe104116f97e693c6b Make functions available to be moved later on 7594a223af9cdd4a33f6857ecdc22df5ba84ed6b Move checks and package initialization after build d1ac66729e868cf88e21068fbb5ba12c309430ef Remove checks during parsing of packages 5af0ec9694263d4784d4056237d057b5d6977c33 Always start parsing in the preable of the main package 10eb812919f95f6c1a14d4261956d5b1217f279e Add test case for dynamic spec -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646/files/ab419377d037e5604f90ba911f114b044c67ad47..10eb812919f95f6c1a14d4261956d5b1217f279e You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> @@ -21,6 +23,13 @@ echo "Q: Why?\nA: Because we can!" > FAQ
mkdir -p $RPM_BUILD_ROOT/usr/local/bin
echo " " > $RPM_BUILD_ROOT/usr/local/bin/hello
+%{?FULLDYNAMIC:
+echo "Group: Utilities" >> %{specpartsdir}/docs.specpart
+echo "License: GPL" >> %{specpartsdir}/docs.specpart
+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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1670244279
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> @@ -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
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1670214549
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> +rpmRC rc = RPMRC_FAIL;
+
+/* XXX Skip valid arch check if not building binary package */
+if (!(spec->flags & RPMSPEC_ANYARCH) && checkForValidArchitectures(spec)) {
+ goto exit;
+}
+
+fillOutMainPackage(spec->packages->header);
+/* Define group tag to something when group is undefined in main package*/
+if (!headerIsEntry(spec->packages->header, RPMTAG_GROUP)) {
+ headerPutString(spec->packages->header, RPMTAG_GROUP, "Unspecified");
+}
+
+char *platform = rpmExpand("%{_target_platform}", NULL);
+char *os = rpmExpand("%{_target_os}", NULL);
+char *optflags = rpmExpand("%{optflags}", NULL);
Okay, rebase to get the warnings -> compile fail on CI.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#discussion_r1352405865
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> +rpmRC rc = RPMRC_FAIL;
+
+/* XXX Skip valid arch check if not building binary package */
+if (!(spec->flags & RPMSPEC_ANYARCH) && checkForValidArchitectures(spec)) {
+ goto exit;
+}
+
+fillOutMainPackage(spec->packages->header);
+/* Define group tag to something when group is undefined in main package*/
+if (!headerIsEntry(spec->packages->header, RPMTAG_GROUP)) {
+ headerPutString(spec->packages->header, RPMTAG_GROUP, "Unspecified");
+}
+
+char *platform = rpmExpand("%{_target_platform}", NULL);
+char *os = rpmExpand("%{_target_os}", NULL);
+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...
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#discussion_r1352328930
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> +rpmRC rc = RPMRC_FAIL;
+
+/* XXX Skip valid arch check if not building binary package */
+if (!(spec->flags & RPMSPEC_ANYARCH) && checkForValidArchitectures(spec)) {
+ goto exit;
+}
+
+fillOutMainPackage(spec->packages->header);
+/* Define group tag to something when group is undefined in main package*/
+if (!headerIsEntry(spec->packages->header, RPMTAG_GROUP)) {
+ headerPutString(spec->packages->header, RPMTAG_GROUP, "Unspecified");
+}
+
+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);
| ^~~~
```
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1667419879
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1755155729 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> @@ -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
:sweat_smile:
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1667375999
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1755114032 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1746599449 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. Or %prep/%build/%install? And that must be just scratching the surface of such issues... -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1746477484 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1744366996 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@ffesti pushed 6 commits. 7ec8dd2235d6d684f590d3bb92ec6adfa75d583d Drop NVR parameter to make them easier to reuse 65be4c1e70b7b7745b582dc59be9de1832f550d7 Make functions available to be moved later on 1894a121973c1f93decc64e6a81a9a790d9288a3 Move checks and package initialization after build 04bad44bd3f77f73b5d7aa11edd04aefb7d5ebd6 Remove checks during parsing of packages 8d68e4cc7c11a248f835eb9b01fab72d45650a58 Always start parsing in the preable of the main package ab419377d037e5604f90ba911f114b044c67ad47 Add test case for dynamic spec -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646/files/82cb2da422db2f86aa3a81e5cfc9537ce4ab8e9f..ab419377d037e5604f90ba911f114b044c67ad47 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1735416841 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai commented on this pull request.
> @@ -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.
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1644067342
You are receiving this because you are subscribed to this thread.
Message ID: ___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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? -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1735278819 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1735266779 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
Actually, please drop the move commit out of this set. That's what makes so unrevieable on GH, and that's not even an interesting commit in itself :sweat_smile: -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1735250389 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
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. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1735245703 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@ffesti pushed 6 commits. 28dbca6a15efdef33863c1412b331279e9ae2853 Drop NVR parameter to make them easier to reuse fb0d81ef9cb886ec31e34c64b13ade85e023b062 Make functions available to be moved later on 7d3d23cd7c13de952be577f47dfad5bfd0bdda74 Move checks and package initialization after build 1f904e5ef45cd77f7ed57341129b16f22d5de10b Remove checks during parsing of packages 3cd75637989d459a13ba1552f674af9f2024265a Move functions from parsePreamble to parseSpec 6b381f14cc17a8ba193f951e2df730d4b8b7836e Always start parsing in the main package -- View it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646/files/0d6594736b8978cc1c5a7af98d3c1c95b00c156a..6b381f14cc17a8ba193f951e2df730d4b8b7836e You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
This is the major part of what is needed for #1240 -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#issuecomment-1718917225 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] Move checks and package initialization after build (PR #2646)
@pmatilai requested changes on this pull request. Eek. I can see why you want to do something like this, but this kind of mega-patch that 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, which becomes actually revievable now, and finally do the actual move of the those unchanged helpers in a commit that only moves and doesn't change. -- Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/2646#pullrequestreview-1610800180 You are receiving this because you are subscribed to this thread. Message ID: ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
