-
Notifications
You must be signed in to change notification settings - Fork 375
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
No build directives in generated spec parts #2917
Conversation
build/parseSpec.c
Outdated
return p->beforebuildonly; | ||
} | ||
} | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd combine these two into findPart() that returns a pointer to the struct, whose values you can then just use directly in the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still this one 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I wrote above this is not quite as simple as it looks and I ma not that interested in refactoring the whole parting code. Yes, it looks wasteful and over complicated but I'd rather not change a whole bunch of functions just to pass a new pointer around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see you mentioned this below. isPart() is a different matter, but ..
Once you have
static const struct PartRec * getPart(int part)
{
const struct PartRec *p;
for (p = partList; p->token != NULL; p++) {
if (p->part == part) {
return p;
}
}
return NULL;
}
...the check becomes something like:
if (stage == PARSE_GENERATED) {
const struct PartRec *p = getPart(parsePart);
if (p && p->prebuildonly) {
rpmlog(RPMLOG_ERR,
_("Section %s is not allowed after build is done!\n"),
p->token);
goto errxit;
}
}
...and at that point you realize this whole if belongs into a separate helper function that checks whether the tag was legitimate for use here. parseSpecSection() is big enough as it is.
It's not exactly a huge refactoring job 😅 I mean, if you like, just stick the whole logic into one function you call from parseSpecSection(). But having two separate lookup functions just for this is a bit silly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the new code is that more readable. But whatever...
As we now have three different sources of spec content we need a proper enum to tell them apart and get rid of the seconday bool parameter. No functional change but is needed for the next patch
Oh and for whatever reason, there's some po/ submodule update in here:
(also visible in the changed files tab) |
Many things need to be known before the build can be started. Make declaring these sections or directives an error when encountered parsing the generated Spec parts. Resolves: rpm-software-management#2693
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually meant you could just as well inline the whole lookup in what you're calling checkPart() since there are no other users for this in sight, but works for me.
Many things need to be known before the build can be started. Make
declaring these sections or directives an error when encountered parsing
the generated Spec parts.
Resolves: #2693