Pin libraries to current releases #298
Conversation
|
I was originally going to open an issue about the fact that the curl demo couldn't build/run on macOS without the |
ajbozarth
added
bug
|
@ajbozarth Thanks for the fix to the |
|
The curl/README.md should be updated with any new arguments, and the change in defaults |
|
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 run It could be helpful to at least have the default build example using the correct tag perhaps |
@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.
@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 |
|
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.
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) |
ajbozarth
changed the title
|
As promised in #284 (comment) I have just pushed my update for the nginx demo and added a task list to the top description for tracking my progress in updating demos. I will also be updating the circleCI for the 3 jobs currently being run (openssl3, nginx, and httpd). As I will be pushing updated here periodically as I finish them, I will ping again for review when I have finished. Feel free to either wait until then or review as I go at your leisure. |
There was a problem hiding this comment.
Thanks for these updates @ajbozarth, FYI - I've deployed the nginx test-server update at test.openquantumsafe.org and all tests pass.
Status UpdateFully updated demos
Updated demo (with issues)
Still to be worked on
Personal notesI believe the work here is getting close to done. I plan to bring up some of my notes above at next Tuesday's meeting to discuss what "done" is here. After the discussion with the team I plan to start looking at #294 as a follow up to this work. |
|
ngtcp2 should be able to build with boringssl |
Supporting building with boringssl is a great idea, but the current demo doesn't support it. So I would consider that a new feature that should be handled in a separate PR |
|
Thanks for the summary, @ajbozarth . Regarding haproxy, may I suggest @pi-314159 and @davidkel sync? IMO all that's needed there is a link to the haproxy documentation as per #273 (comment). Unless of course you want to keep supporting a docker image despite the upstream apparently already supporting oqs (?). Sounds a bit like wasted effort, no? |
I get what you mean, I'll discuss with @davidkel and see if he still wants to update if it's just "putting the doc steps into a Dockerfile" I'll also raise the topic of just dropping it in Tuesday's meeting (I already planned on discussing dropping at least unbound since it's out of date and there was no interest in it). |
|
Sharing plans based on todays discussion on the OQS call: Demo specific updates:
Follow up work:
|
|
FYI X11 is still working for me on mac sequoia (I have XQuartz installed) and Fedora core 41 .. will try demo |
"Unsupported" but CI-supported -- sounds weird. Can I suggest the terms |
This is much better, thank you
We did not discuss what to do with already published docker images for deprecated demos, In my opinion as long as those published docker images still run as expected there no reason to delete them.
I had intended to run all of the against main/master, but when I get to the CI work we can discuss if we only want to test the maintained demos and not unmaintained against main/master |
|
I tried epiphany on fedora workstation 41 (where I can run X11 apps) I noticed the launch invocation seems incorrect in that it does not pass $DISPLAY. When I do so I get: which is some issue relating to the accessibility bus. The main app window starts to draw, then the app exits. However I don't think this is necessarily worth pursuing |
|
I would suggest we update the textual description for each image published on dockerhub. In particular I agree we don't need to remove old demos, but we should at least clearly add a note to the description that they are maintained or deprecated, as well as ensure there's a link back to the relevant directory in this source repository. |
|
Per my above comments I have pushed the following:
This leaves two open action items:
Point of discussion - Wireshark@davidkel brought to my attention that Wireshark already added a bunch of PQ support a while back (see here), given the overhead for updating the wireshark demo and the existing support I decided to mark it deprecated. If there is push back on this I suggest we open a follow up issue to "un-deprecate it" and some other brave soul can pick it up. |
|
Was looking through the issue backlog while waiting on a openlitespeed build and the expanded work in the PR might mean closing a handful of open issue with the PR:
If there's no issues with my above assertions I will add |
|
After some effort trying to update the openlitespeed demo, I'm hit too many build issues and don't have the expertise on openlitespeed to sort them out. As there was no interest in the openlitespeed demo to begin with I am marking it as deprecated instead. If there's interest I can still push the updates I made that aren't building if someone else wants to take a shot. They are currently sitting in a separate branch on my fork, where I'll leave it for future reference ajbozarth@24f81fb |
|
As mentioned above I've added a I also opened #312 as a follow up issue to handle the creation of a As mentioned I left a comment on #255 since I did not include it in the Fixes list despite deprecating wireshark. Lastly @SWilson4 did you have any luck testing epiphany? Because if it does not work for you I plan to do the following:
|
|
Worth noting here: I just found out that openssl-3.4.0 was release last week. I'd rather not block the merge of this PR, instead I would like to wait until my CI updates and take the opportunity to use the update to openssl-3.4.0 to test them |
|
I get a similar error to @planetf1 when I try to run epiphany on Ubuntu 24. |
In that case I'll take some time today to go through the steps I listed above to deprecate epiphany. I also noticed CI complaining about my lack of commit sign-offs for my more recent commits, I'll address that at the same time |
|
I've taken the steps to deprecate epiphany, and updated the relevant docs and GitHub issues. With this, the PR is now in a state of final review. I have no more outstanding work items or review items to address prior to merging. The only caveat is the DCO CI failing due to my forgetting to sign off on my later commits. Remediating this would be messy due to multiple merges with master. I could do a squash and force push if it is an actual blocker, but afaik every PR is merged with a "Squash and merge" so is it necessary? If so I will need to create a new branch to hold the epiphany commit that's referenced in its deprecation message since a squash and force would delete the referenced url. @baentsch you usually handle merges in this repo. |
|
As an FYI in case it's useful in the future for @ajbozarth or others, you can automate signing off on commits for a repo by following the (very well hidden) instructions in |
Thanks, I knew I could automate it, but this is the first project I've worked on that required sign offs so I hadn't taken the time to figure out how to do so |
|
A few updates per discussion in todays call:
After this PR has final approval I will run a giant git squash with sign-off to fix the DCO failures as per todays discussion (unless @ryjones you have better solution?) |
You could to a |
There was a problem hiding this comment.
Thanks for this work @ajbozarth -- this is a substantial improvement over the previous state of things. One thing did deteriorate, though, namely the breadth of algorithms implicitly tested by the different integrations: It was intentional to not always use kyber768 but to also use other algs. Please consider bringing back this diversity -- unless there are strong reasons to do something different.
Could point out the specific cases where this changed? I had not intended to touch the algorithms used unless it was breaking the build, like switching to mldsa in the openssh demo. On a quick reread it does look like @davidkel did touch algorithms in his changes (haproxy and mosquitto). @baentsch If you want me to try and address that I can otherwise I'll do the squats and force push previously discussed later today |
Updates demos to use a pinned release version rather than main/master Updates demo builds to support both linux/amd64 and linux/arm64 Deprecates demos that were unable to be updated due to any reason haproxy and mosquitto demo updates provided by David Kelsey Co-authored-by: Dave Kelsey <[email protected]> Signed-off-by: Alex Bozarth <[email protected]>
|
I have pushed the giant squashed commit with an updated commit message. I also copied the previous git history into a new branch on my fork and protected it like I did with the epiphany and openlitespeed updates. This will allow at least some easy reference for future devs looking at the PR for help. As discussed with @baentsch in todays OQS call I will be making sure that my test cases I add in my CI work include more algorithm diversity that was lost in this update (specifically haproxy and mosquitto) This PR should be good to merge now, just double check the Issues it will be automatically closing is correct first. |
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
Fixes #230 Fixes #182 Fixes #273 Fixes #226 Fixes #171
Demos updated (ordered by interest):
chromiumN/AquicN/A