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
@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
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
@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
@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
@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" >>
@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
@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;
+}
+
+
@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;
+}
+
+
@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;
+}
+
+
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
@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
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
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
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
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:
@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
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
@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.
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
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
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:
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
@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
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:
@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
26 matches
Mail list logo
