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

(#14620) libzip: Use robust github mirror #14619

Merged
merged 1 commit into from
Dec 7, 2022
Merged

(#14620) libzip: Use robust github mirror #14619

merged 1 commit into from
Dec 7, 2022

Conversation

miklelappo
Copy link
Contributor

@miklelappo miklelappo commented Dec 7, 2022 •
edited
Loading

https://libzip.org is down
Use github mirror for releases.
The difference in SHA-s is related to .github directory and .clang-format file, which are present in github release.

The source was compared to original used by conan to avoid potential attack by getting libzip.org down and fak-ing github sources

Specify library name and version: libzip/all

Closes #14620

This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!

@miklelappo
Copy link
Contributor Author

miklelappo commented Dec 7, 2022 •
edited
Loading

@ericLemanissier @uilianries this is a potential show-stopper for all projects using libzip via Conan. Should the build pass, can we please get this reviewed soon? I'll open a bug ticket...

Thanks a lot for your help!

@miklelappo miklelappo changed the title (#xxxxx) libzip: Use robust github mirror (#14620) libzip: Use robust github mirror Dec 7, 2022
@ericLemanissier
Copy link
Contributor

Does https://github.com/nih-at/libzip/releases/download/v1.9.2/libzip-1.9.2.tar.gz have the same sha? Please add mirror instead of replacing existing source

@miklelappo
Copy link
Contributor Author

@ericLemanissier it doesn't.... I'll post a screenshot why

@mlappo
Copy link

mlappo commented Dec 7, 2022

image
left what we used before, right what released on github...

@github-actions github-actions bot mentioned this pull request Dec 7, 2022
Open
@conan-center-bot

This comment has been minimized.

Sign in to view
ericLemanissier
ericLemanissier previously approved these changes Dec 7, 2022
View reviewed changes
@miklelappo
Copy link
Contributor Author

Thanks a lot @ericLemanissier! I hope we can get this in very soon....

AntonRechkov
AntonRechkov previously approved these changes Dec 7, 2022
View reviewed changes
Croydon
Croydon previously approved these changes Dec 7, 2022
View reviewed changes
@jcar87
Copy link
Contributor

jcar87 commented Dec 7, 2022

Thanks @miklelappo for troubleshooting and fixing this one.

Is the original file with the original file available anywhere? That is, in the screenshot with the before and after, where is the "before" coming from if libzip.org is down?

If the original file with the original file is available, out of an abundance of caution (based on past experience), I'd like to make sure that the checksums of each individual file inside the package matches was was there previously. We have seen instances in the past where even for the same release number, the contents do change in a way that impacts behaviour seen by consumers.

jcar87
jcar87 requested changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

libzip.org doesn't seem to be down anymore and we require additional verification to handle checksum/URL changes - please bear with us :)

@mlappo
Copy link

mlappo commented Dec 7, 2022 •
edited
Loading

@jcar87, libzip.org went back up after almost a day. I guess we still want the change in, because github is more reliable in future, right?

The "before" is coming from conan cache.
The "after" is coming from github release.

I guess meld is doing required checksuming, but we can make it again.
Would you like to do it yourself or me to make this?

@jcar87
Copy link
Contributor

jcar87 commented Dec 7, 2022

@jcar87, libzip went back up after almost a day. I guess we still want the change in, because github is more reliable in future, right?

GitHub itself as a whole probably, but we need to be careful about the tarballs that are served via the /archive endpoint. My understanding is that they are not checksum stable, which has caused issues for package maintainers in the past. My current understanding is that it is still the case that this is not guaranteed - they just happen to have been stable for a while (happy to be corrected on this, in case GitHub now has a documented policy on this).

On the other hand, I can see that this project has Releases on its GitHub page, with the URL for 1.8.0 being:
https://github.com/nih-at/libzip/releases/download/v1.8.0/libzip-1.8.0.tar.gz

Notice how this is in /releases rather than /archive - in this case, I believe it is guaranteed that GitHub will serve the exact same file over time. So in any case, this URL would be preferable.
I can see that the tarball served by the release has the same checksum as the original one. So we could fix the conandata.yml to something like this:

    url: [
      "https://libzip.org/download/libzip-1.8.0.tar.gz",
      "https://github.com/nih-at/libzip/releases/download/v1.8.0/libzip-1.8.0.tar.gz",
    ]
    sha256: "30ee55868c0a698d3c600492f2bea4eb62c53849bcf696d21af5eb65f3f3839e"

This gives us some redundancy, while preserving the checksums the recipe had already been built against :)

@miklelappo
Copy link
Contributor Author

miklelappo commented Dec 7, 2022 •
edited
Loading

All right!
My bad... took the wrong link in rush ;)

@miklelappo
(#xxxxx) libzip: Use robust github mirror
1736e0b 
https://libzip.org is down
Use github mirror for releases.
@miklelappo
Copy link
Contributor Author

@jcar87 fixed.

@jcar87
Copy link
Contributor

jcar87 commented Dec 7, 2022

@jcar87 fixed.

Thanks so much! :)

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 3 (1736e0b50a0124f64cd418b7c188b2faa0904233):

  • libzip/1.8.0@:
    All packages built successfully! (All logs)

  • libzip/1.7.3@:
    All packages built successfully! (All logs)

@miklelappo miklelappo requested review from ericLemanissier and Croydon and removed request for Croydon and ericLemanissier December 7, 2022 18:18
@prince-chrismc
Copy link
Contributor

Nice catch eric 👍

Seems like teamwork makes the dream work

@prince-chrismc
Copy link
Contributor

Notice how this is in /releases rather than /archive - in this case, I believe it is guaranteed that GitHub will serve the exact same file over time. So in any case, this URL would be preferable.

Those files are "manually" selected and picked by the project maintainers in my experience so those are definitely way better

@ericLemanissier
Copy link
Contributor

I think the sha is wrong for github's release link. These also have .github folder and clang-format file

@miklelappo
Copy link
Contributor Author

@ericLemanissier I just checked locally by removing non-github paths and sha-ing seems to work for me

@miklelappo
Copy link
Contributor Author

image
For me it definitely works @ericLemanissier

@miklelappo miklelappo requested review from AntonRechkov and removed request for Croydon December 7, 2022 19:02
@conan-center-bot conan-center-bot merged commit 2ecc63a into conan-io:master Dec 7, 2022
@ericLemanissier
Copy link
Contributor

@miklelappo I don't understand where your screenshot above comes from. The files I get from https://libzip.org/download/libzip-1.8.0.tar.gz and https://libzip.org/download/libzip-1.7.2.tar.gz have .github folder and clang-format file

@miklelappo
Copy link
Contributor Author

ah, you mean the very early screenshot from today morning? This was unpacked Conan-cache on my working machine when libzip.org was down... there I didn't have that files.

Good, but we don't have concerns that links you posted above (https://libzip.org/...) has incorrect checksums?

@miklelappo
Copy link
Contributor Author

miklelappo commented Dec 7, 2022 •
edited
Loading

What is important that links we have right now point to same file with same sha...
image

and as @jcar87 correctly pointed out, the archive one has a different checksum
image

@miklelappo miklelappo deleted the chore/libzip/down branch December 7, 2022 19:19
@ericLemanissier
Copy link
Contributor

I have no concern about the correction as it is now. This is exactly what I suggested (not clearly enough) in #14619 (comment). I'm confused because of the screenshot which does not make sense.

@miklelappo
Copy link
Contributor Author

Understood. Sorry for confusion... I didn't know the difference between archive and release source provided by GitHub... Need to take more look into that

datalogics-robb pushed a commit to datalogics-robb/conan-center-index that referenced this pull request Mar 6, 2023
@datalogics-kam
Merge branch 'develop' of octocat.dlogics.com:datalogics/conan-center…
652f00d 
…-index

* 'develop' of octocat.dlogics.com:datalogics/conan-center-index: (6046 commits)
  cmake: Remove the private tag from the openssl requirement
  (conan-io#14689) doctest: Use self.info.clear() instead of header_only()
  (conan-io#14684) imath: add version 3.1.6
  (conan-io#14679) tgbot: add version 1.5
  (conan-io#14672) luau: add version 0.556
  (conan-io#14673) fast_double_parser: add version 0.7.0
  (conan-io#14664) sqlite_orm: add version 1.8
  (conan-io#14663) magic_enum: add version 0.8.2
  (conan-io#14658) Update changelog 09-December-2022
  (conan-io#14525) Add Boost.LEAF to Conan Center
  (conan-io#13722) Add wavelet_buffer v0.4.0
  (conan-io#14655) etl: add version 20.35.5
  (conan-io#14654) nss 3.86
  (conan-io#14652) flatbuffers: add version 22.12.06
  (conan-io#14427) libxml2: fix CMake vars in CMakeDeps & bump icu
  (conan-io#14626) [googleapis] Use is_msvc to abstract away compiler name setting
  (conan-io#14619) (conan-io#14620) libzip: Use robust github mirror
  (conan-io#14617) pybind11_json: add version 0.2.13
  (conan-io#14476) add libhydrogen/cci.20221115
  (conan-io#13917) cimg: conan v2 support + bump dependencies + disable dependencies by default
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcar87 jcar87 jcar87 approved these changes

@prince-chrismc prince-chrismc prince-chrismc approved these changes

@Croydon Croydon Croydon left review comments

@ericLemanissier ericLemanissier ericLemanissier left review comments

@AntonRechkov AntonRechkov Awaiting requested review from AntonRechkov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[package] libzip/all: https://libzip.org is down
8 participants
@miklelappo @ericLemanissier @mlappo @conan-center-bot @jcar87 @prince-chrismc @Croydon @AntonRechkov