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

Pin libraries to current releases in curl demo #298

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ajbozarth
Copy link
Member

Sets the default tag for openssl, liboqs, oqs-provider to the current latest relese instead of main/master. Also updates curl to the latest release.

Inlcudes some fixes to support multi-platform builds, now supporting both linux/amd64 and linux/arm64, where only linux/amd64 worked before.

Related Issues: #230 #213 #182

Sets the default tag for openssl, liboqs, oqs-provider to the current
latest relese instead of main/master. Also updates curl to the latest
release.

Inlcudes some fixes to support multi-platform builds, now supporting
both linux/amd64 and linux/arm64, where only linux/amd64 worked before

Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth
Copy link
Member Author

I was originally going to open an issue about the fact that the curl demo couldn't build/run on macOS without the --platform linux/amd64 flag, but as I worked on this changes I made solved that issue. Also #213 seems to partially cover that issue

@ajbozarth ajbozarth self-assigned this Sep 18, 2024
@ajbozarth ajbozarth added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Sep 18, 2024
@baentsch
Copy link
Member

@ajbozarth Thanks for the fix to the lib64 installpath. What's missing in my view is testing (CI) for this: I'd suggest two runs (via GH CI): One building the "main/master" image as "canary" for OQS; the second one the "version pegged" image for user consumption. Of course only the latter should be pushed to docker hub during merge. And also, it should be a cross-platform image, thus truly testing x64 and arm64 "suitability". Thanks.

@ajbozarth ajbozarth marked this pull request as ready for review September 24, 2024 16:59
curl/Dockerfile Outdated Show resolved Hide resolved
curl/Dockerfile Show resolved Hide resolved
curl/Dockerfile Show resolved Hide resolved
@planetf1
Copy link

The curl/README.md should be updated with any new arguments, and the change in defaults
(Rather than stating explicit version it could just say 'updated to the most recent versions that have been tested' or similar).

@planetf1
Copy link

Noticed that text in README.md refers to how to build the image, and provides a few examples. But the tag varies.

Once we get to USAGE.md it states to rundocker run -it openquantumsafe/curl perftest.sh- so by default this will run the version from dockerhub.

It could be helpful to at least have the default build example using the correct tag perhaps docker build . -t open-quantum-safe/curl

@ajbozarth
Copy link
Member Author

What's missing in my view is testing (CI) for this

@baentsch After our discussion during this week's sync up I took another look and realized I had only investigated the GitHub Actions and not circleci, leading me to think I would have to write tests from scratch. Which I evaluated would be better done in a follow up PR, but upon finding the circleci test configs I will make updates there. Though as #294 says, we should move our circleci automation to gh actions in a future PR.

But the tag varies

@planetf1 I will take a look through the docs, and make sure the tag it uses (docker hub or local build) matches the context of the section it's in. If it doesn't or is unclear I will made the edit or add clarification to the surrounding text

Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth
Copy link
Member Author

ajbozarth commented Sep 25, 2024

I just pushed an update addressing @planetf1 inline comments, and adding a couple lines to the circleCI to build and test the curl image using main/master libs as well as the pinned ones.

@planetf1 I looked into the docs for inconsistencies in the build image name and found that is very consistent, using the local image name in the README and using the docker hub image in USAGE. It also includes a line at the top of USAGE clarifying that it uses the public image name throughout that doc.

The curl/README.md should be updated with any new arguments

I updated the README to include the new ARGs and removed direct reference to versions so as any future updates to the pinned version would not require updates to the docs

Lastly, on the CI update, what I pushed was a simple addition since we should add more through tests when we move testing to GH Actions, which currently only does builds, and only for one demo. (covered by #284 IIUC)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants