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

[libzypp] Do not disable libproxy use. JB#56759 #2

Closed
wants to merge 1 commit into from
Closed

[libzypp] Do not disable libproxy use. JB#56759 #2

wants to merge 1 commit into from

Conversation

martyone
Copy link
Member

No description provided.

@martyone martyone requested a review from mkosola March 15, 2022 08:56
@martyone
Copy link
Member Author

Despite this -DDISABLE_LIBPROXY=TRUE, which was added during upgrade to 17.24.2 with commit 7be7f21, there is also BuildRequires: pkgconfig(libproxy-1.0) left from past. So I guess the option appeared as new during that upgrade, which was wrongly perceived as that the feature to use libproxy was new and so it was disabled as unneeded, missing the fact that it was actually used before.

saukko
saukko reviewed Mar 15, 2022
View reviewed changes
rpm/libzypp.spec
@@ -94,7 +94,6 @@ export CXXFLAGS="%{optflags}"
-DCMAKE_BUILD_TYPE=Release \
-DCMAKE_SKIP_RPATH=1 \
-DUSE_TRANSLATION_SET=${TRANSLATION_SET:-zypp} \
-DDISABLE_LIBPROXY=TRUE \
Copy link
Contributor

Choose a reason for hiding this comment

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

If this would be =FALSE would it enforce this always and fail in case someone removes libproxy?

Copy link

Choose a reason for hiding this comment

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

Not it doesn't:
https://github.com/sailfishos-mirror/libzypp/blob/43bd2bf563be01d429e7930019da7682ef430c1c/CMakeLists.txt#L242

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately it doesn't work that way

@Thaodan
Copy link

Thaodan commented Mar 15, 2022

Despite this -DDISABLE_LIBPROXY=TRUE, which was added during upgrade to 17.24.2 with commit 7be7f21, there is also BuildRequires: pkgconfig(libproxy-1.0) left from past. So I guess the option appeared as new during that upgrade, which was wrongly perceived as that the feature to use libproxy was new and so it was disabled as unneeded, missing the fact that it was actually used before.

Please add this to the commit message as context.

@martyone
Copy link
Member Author

I found that it would be more clear to have a separate bug for this, so replaced with PR #3 .

@Thaodan I extended the commit message there.

@martyone martyone closed this Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saukko saukko saukko left review comments

@Thaodan Thaodan Thaodan left review comments

@mkosola mkosola Awaiting requested review from mkosola

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@martyone @Thaodan @saukko