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

Implement processing of conditions #276

Merged
merged 3 commits into from
Aug 22, 2023
Merged

Conversation

nforro
Copy link
Member

@nforro nforro commented Aug 20, 2023

This is not a full support for conditions, i.e. it doesn't re-solve #231 where an entire section can be conditionalized (however, the new process_conditions() function is generic enough to be used as a base for the resolution of that issue), nevertheless I think it's quite an improvement.

Note that macro definitions have to be parsed twice, because a macro definition can contain a condition (see https://src.fedoraproject.org/rpms/kernel/blob/rawhide/f/kernel.spec for examples), and vice versa, a condition can encapsulate a macro definition.

Macro definitions and tags gained a valid attribute that can be used to determine if that particular macro definition/tag is valid and can affect other entities in the spec file or if it would be ignored when parsing the spec file with RPM. This will be used to fix Specfile.update_value() and Specfile.update_tag(), but it could be beneficial for other use cases as well.

Related to packit/packit#2033.

RELEASE NOTES BEGIN

Macro definitions and tags gained a new valid attribute. A macro definition/tag is considered valid if it doesn't appear in a false branch of any condition appearing in the spec file.

RELEASE NOTES END

@lachmanfrantisek
Copy link
Member

Note that macro definitions have to be parsed twice

Are there any performance concerns? (E.g. in Copr)

@nforro
Copy link
Member Author

nforro commented Aug 21, 2023

Are there any performance concerns? (E.g. in Copr)

Well, it will make things slower, but it shouldn't be noticeable. Parsing texlive.spec takes roughly 100ms more (that's 100ms out of 9.5 seconds).

Copy link
Member

@lachmanfrantisek lachmanfrantisek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wow, this really shows what it means to parse the specfile..;-D

I am only not sure if I am able to review this..;) So, just a few code-style nit picks.

specfile/conditions.py Outdated Show resolved Hide resolved
specfile/conditions.py Outdated Show resolved Hide resolved
@nforro nforro added mergeit Zuul, merge it! and removed mergeit Zuul, merge it! labels Aug 21, 2023
@nforro
Copy link
Member Author

nforro commented Aug 22, 2023

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/114fea4049074c34b0de7b5c61975e0b

✔️ pre-commit SUCCESS in 1m 23s
✔️ specfile-tests-rpm-deps SUCCESS in 1m 05s
✔️ specfile-tests-pip-deps SUCCESS in 1m 00s

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).
https://softwarefactory-project.io/zuul/t/packit-service/buildset/6b516068e2e14fcc99fc5595fd9453ca

✔️ pre-commit SUCCESS in 1m 22s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 5781f2f into main Aug 22, 2023
2 of 26 checks passed
@delete-merged-branch delete-merged-branch bot deleted the conditions branch August 22, 2023 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mergeit Zuul, merge it!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants