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.

Sign up for GitHub

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

Jump to bottom

builddep: Don't try to expand globs in pkg specs #1085

Merged
merged 2 commits into from
Mar 15, 2024
Merged

builddep: Don't try to expand globs in pkg specs #1085

merged 2 commits into from
Mar 15, 2024

Conversation

evan-goode
Copy link
Member

@evan-goode evan-goode commented Dec 12, 2023 •
edited
Loading

This commit makes a breaking change adding expand_globs setting to the ResolveSpecSetting.

Related: rpm-software-management/mock#1267
Resolves #1084

Are there any other places where the expand_globs setting should be false? Or maybe false should be the default and the commands that accept pkg specs on the command line should set it to true themselves?

WIP since a ci-dnf-stack PR is still on the way.

@evan-goode evan-goode requested a review from m-blaha December 12, 2023 01:46
@evan-goode evan-goode self-assigned this Dec 12, 2023
@m-blaha
Copy link
Member

m-blaha commented Dec 12, 2023

We definitely need this feature.
But since this is an ABI-breaking change that we're not ready to deliver quickly, I was thinking about working around the problem on the builddep side. Would escaping [ (and potentially other globs) before calling goal->add_rpm_install() help?

@praiskup
Copy link
Member

Would escaping [ (and potentially other globs) before calling goal->add_rpm_install() help?

+1

There just seem to be 3 of them.

evan-goode added a commit to evan-goode/dnf5 that referenced this pull request Dec 12, 2023
@evan-goode
builddep: Escape glob characters in pkg specs
2b7da7b 
Not parsing the pkg specs as globs would require an ABI-breaking change
(rpm-software-management#1085). A
workaround that doesn't involve breaking changes is to escape the glob
characters in the pkg specs.

Related: rpm-software-management/mock#1267
Resolves rpm-software-management#1084
This was referenced Dec 12, 2023
Merged
Merged
@evan-goode
Copy link
Member Author

We definitely need this feature. But since this is an ABI-breaking change that we're not ready to deliver quickly, I was thinking about working around the problem on the builddep side. Would escaping [ (and potentially other globs) before calling goal->add_rpm_install() help?

Yes, that seems to work. I opened a new PR that does it that way: #1088.

ci-dnf-stack PR: rpm-software-management/ci-dnf-stack#1425

@kontura kontura mentioned this pull request Dec 13, 2023
Closed
10 tasks
evan-goode added a commit to evan-goode/dnf5 that referenced this pull request Dec 13, 2023
@evan-goode
builddep: Escape glob characters in pkg specs
78a462f 
Not parsing the pkg specs as globs would require an ABI-breaking change
(rpm-software-management#1085). A
workaround that doesn't involve breaking changes is to escape the glob
characters in the pkg specs.

Related: rpm-software-management/mock#1267
Resolves rpm-software-management#1084
github-merge-queue bot pushed a commit that referenced this pull request Dec 14, 2023
@evan-goode @m-blaha
builddep: Escape glob characters in pkg specs
a4662a7 
Not parsing the pkg specs as globs would require an ABI-breaking change
(#1085). A
workaround that doesn't involve breaking changes is to escape the glob
characters in the pkg specs.

Related: rpm-software-management/mock#1267
Resolves #1084
@jan-kolarik
Copy link
Member

We definitely need this feature. But since this is an ABI-breaking change that we're not ready to deliver quickly, I was thinking about working around the problem on the builddep side. Would escaping [ (and potentially other globs) before calling goal->add_rpm_install() help?

Yes, that seems to work. I opened a new PR that does it that way: #1088.

ci-dnf-stack PR: rpm-software-management/ci-dnf-stack#1425

So is this PR still needed? Either we should close it or target it against the dnf-5.2.0.0 branch.

@evan-goode evan-goode changed the base branch from main to dnf5-5.2.0.0 January 30, 2024 19:41
@evan-goode evan-goode force-pushed the evan-goode/builddep-no-glob branch 2 times, most recently from 6268a0d to e8579c6 Compare January 30, 2024 21:10
@evan-goode
Copy link
Member Author

So is this PR still needed? Either we should close it or target it against the dnf-5.2.0.0 branch.

Thanks for the reminder, I think it's still worth doing. Retargeted and rebased against dnf5-5.2.0.0, and got rid of the temporary fix from #1088.

@evan-goode evan-goode marked this pull request as ready for review January 30, 2024 21:10
@evan-goode evan-goode removed their assignment Feb 6, 2024
@kontura kontura force-pushed the dnf5-5.2.0.0 branch 2 times, most recently from 1577163 to 599f7b5 Compare February 28, 2024 08:26
@m-blaha
Copy link
Member

m-blaha commented Mar 7, 2024

The PR seems to be unexpectedly big and contains unrelated commits. Maybe caused by 5.2 re-targetting?

@kontura
Copy link
Contributor

kontura commented Mar 14, 2024

@evan-goode please rebase the PR.

@evan-goode
builddep: Don't try to expand globs in pkg specs
a2af49a 
This commit makes a breaking change adding `expand_globs` setting to the
`ResolveSpecSetting`.

Related: rpm-software-management/mock#1267
Resolves rpm-software-management#1084
@evan-goode
builddep: Don't escape globs, use expand_globs = false
63f3d89
@evan-goode evan-goode force-pushed the evan-goode/builddep-no-glob branch from e8579c6 to 63f3d89 Compare March 14, 2024 22:28
@evan-goode
Copy link
Member Author

@evan-goode please rebase the PR.

Done

@kontura kontura merged commit 063032f into rpm-software-management:dnf5-5.2.0.0 Mar 15, 2024
9 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-blaha m-blaha Awaiting requested review from m-blaha

Assignees
No one assigned
Labels
None yet
Projects
DNF team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

builddep cannot handle python3dist(build[virtualenv])
5 participants
@evan-goode @m-blaha @praiskup @jan-kolarik @kontura