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.

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

New rpmExpandMacrosAt API with location information. #494

Closed
wants to merge 1 commit into from

Conversation

codonell
Copy link

@codonell codonell commented Aug 3, 2018

The macro expansion API lacks location information for warnings.
The simplest way to add this information is to pass location
information to a new macro expansion API and for that location
to be used in subsequence diagonistics.

A new 'struct rpmSpecLocation_s' structure is created to hold the
location information. A new API rpmExpandMacrosAt() is created
which operates exactly like rpmExpandMacros() but takes an
additional first argument of type 'rpmSpecLoc' and uses this
information when printing warnings and errors.

Example warning without the patch:
[carlos@localhost SPECS]$ rpmbuild -bs glibc.spec
warning: Macro %pie needs whitespace before body
Wrote: /home/carlos/rpmbuild/SRPMS/glibc-2.28-3.fc29.src.rpm

Example warning with the patch:
[carlos@localhost SPECS]$ rpmbuild -bs glibc.spec
warning: line 66: Macro %pie needs whitespace before body
Wrote: /home/carlos/rpmbuild/SRPMS/glibc-2.28-3.fc29.src.rpm

Notice with the patch 'line 66' is immediately called out at
the location of the macro being expanded, this allows developers
to immediately find the problem.

A solution of a new API with a new location structure was chosen
to minimize the impact on existing structures, APIs, and information
leakage across library boundaries. For example it might have been
convenient to pass the whole rpmSpec to the macro expansion, but
this would have required making the opaque type visible to the
macro expansion code, and may have resulted in maintenance
difficulties. Lastly, even if we extended rpmMacroEntry to contain
location information, we would still need to pass it from the spec
file parsing down, and so could not avoid some other kind of
interface. In the case of rpmMacroContext, adding the current
location to the structure seemed fragile since the context contains
many macros all which can come from different places. Perhaps the
best solution is to eventually expand rpmMacroEntry to record the
location passed by rpmExpandMacrosAt() in order to use it in the
display and dumping of the macro context.

No regressions on x86_64, tested in Fedora Rawhide by using make
dist, integrating the new tarball into the fedora build and then
testing that out to build glibc.spec.

The macro expansion API lacks location information for warnings.
The simplest way to add this information is to pass location
information to a new macro expansion API and for that location
to be used in subsequence diagonistics.

A new 'struct rpmSpecLocation_s' structure is created to hold the
location information. A new API rpmExpandMacrosAt() is created
which operates exactly like rpmExpandMacros() but takes an
additional first argument of type 'rpmSpecLoc' and uses this
information when printing warnings and errors.

Example warning without the patch:
[carlos@localhost SPECS]$ rpmbuild -bs glibc.spec
warning: Macro %pie needs whitespace before body
Wrote: /home/carlos/rpmbuild/SRPMS/glibc-2.28-3.fc29.src.rpm

Example warning with the patch:
[carlos@localhost SPECS]$ rpmbuild -bs glibc.spec
warning: line 66: Macro %pie needs whitespace before body
Wrote: /home/carlos/rpmbuild/SRPMS/glibc-2.28-3.fc29.src.rpm

Notice with the patch 'line 66' is immediately called out at
the location of the macro being expanded, this allows developers
to immediately find the problem.

A solution of a new API with a new location structure was chosen
to minimize the impact on existing structures, APIs, and information
leakage across library boundaries. For example it might have been
convenient to pass the whole rpmSpec to the macro expansion, but
this would have required making the opaque type visible to the
macro expansion code, and may have resulted in maintenance
difficulties. Lastly, even if we extended rpmMacroEntry to contain
location information, we would still need to pass it from the spec
file parsing down, and so could not avoid some other kind of
interface. In the case of rpmMacroContext, adding the current
location to the structure seemed fragile since the context contains
many macros all which can come from different places. Perhaps the
best solution is to eventually expand rpmMacroEntry to record the
location passed by rpmExpandMacrosAt() in order to use it in the
display and dumping of the macro context.

No regressions on x86_64, tested in Fedora Rawhide by using make
dist, integrating the new tarball into the fedora build and then
testing that out to build glibc.spec.
@pmatilai
Copy link
Member

Ok, I submitted an alternative PR that addresses the issue in a bit wider scope (%includes, file manifests and macro file loads covered too) and doesn't affect the API.
Closing this one, but big thanks for your efforts here, had it not been for this PR I doubt I'd gotten around to do #708 at all.

@pmatilai pmatilai closed this May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants