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

pebble: 2.4.0 -> 2.6.0 #331721

Merged
merged 2 commits into from
Oct 10, 2024
Merged

pebble: 2.4.0 -> 2.6.0 #331721

merged 2 commits into from
Oct 10, 2024

Conversation

afh
Copy link
Member

@afh afh commented Aug 2, 2024

Description of changes

This is a follow up PR for #322331, which was prematurely merged and reverted in #331718.

Open tasks that beed to be addressed prior merging:

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@afh
Copy link
Member Author

afh commented Aug 2, 2024

@m1cr0man any information you could share that would help fixing the tests?
Happy to give this a try with input from you in case you are pressed for time currently 🙂

@afh
Copy link
Member Author

afh commented Aug 2, 2024

Result of nixpkgs-review pr 331721 run on x86_64-linux 1

1 package built:
  • pebble

@m1cr0man
Copy link
Contributor

m1cr0man commented Aug 2, 2024

I haven't looked too deep into it but there is a function called check_issuer in the test suite that tries to read and validate the cert's subject to determine if it is still self signed or not:

if "subject" in line:

We need to find a new mechanism for checking the issuer or otherwise checking if the cert is issued by pebble or self signed. I would go for the easiest solution you can think of. I was considering testing if the issuer was not minica but that may break in the same way if minica also updates in the future.

@afh
Copy link
Member Author

afh commented Aug 2, 2024

Thanks, that's helpful, @m1cr0man, I see what I can do. Might take me a few days though as my time and attention is required elsewhere currently.

@afh
Copy link
Member Author

afh commented Aug 2, 2024

@GrahamcOfBorg test acme

@emilazy emilazy mentioned this pull request Aug 6, 2024
@emilazy emilazy changed the title Update pebble pebble: 2.4.0 -> 2.6.0 Aug 14, 2024
@emilazy
Copy link
Member

emilazy commented Aug 14, 2024

Fixing the PR name to stop @r-ryantm opening new ones.

@afh
Copy link
Member Author

afh commented Sep 2, 2024

@m1cr0man haven't been able to make any progress here and I think the understanding required to address this exceeds my current one. Do you think you could help out here?

@m1cr0man
Copy link
Contributor

m1cr0man commented Sep 2, 2024

Yes I'll try my best to do it this week.

@afh
Copy link
Member Author

afh commented Sep 3, 2024

Much appreciated, @m1cr0man, thank you 🙏

@m1cr0man
Copy link
Contributor

Finally got time to find a solution. I'll hopefully commit it tomorrow if the test passes for the new + old versions of pebble.

@afh
Copy link
Member Author

afh commented Sep 30, 2024

As the project is nearing another release, any chance that this PR might make it into it, @m1cr0man?

@m1cr0man
Copy link
Contributor

Yes, I'm quite hopeful. I apologise for my slowness again. This week is looking good time wise for me.

@afh
Copy link
Member Author

afh commented Sep 30, 2024

Thanks for the update, @m1cr0man, much appreciated 🙂

@m1cr0man
Copy link
Contributor

m1cr0man commented Oct 2, 2024

Finally got a PR up :) #346023

I found a relatively simple way to do it in the end. I'm somewhat ashamed it took me a month to make a 10 line change but it is what it is.

@afh
Copy link
Member Author

afh commented Oct 3, 2024

Time to rejoice! Thanks so much, @m1cr0man, for coming up with a fix for the tests!

No need to feel ashamed about a small change some time; we all have our obligations that need our attention, even if we'd like to dedicate ourselves to other matters.

What's the best way to move forward with this PR and your fix?
Can they merged separately?
Should your fix be merged first or into this PRs merge branch?

@flokli
Copy link
Contributor

flokli commented Oct 3, 2024

@m1cr0man with #346023 merged, can you rebase this on top of master?

@afh afh marked this pull request as ready for review October 3, 2024 09:54
@afh
Copy link
Member Author

afh commented Oct 3, 2024

Sure thing, @flokli, this PR branch is now rebased on top of master.

@emilazy
Copy link
Member

emilazy commented Oct 3, 2024

@ofborg test acme

@flokli
Copy link
Contributor

flokli commented Oct 3, 2024

@m1cr0man can you take a look at the failure for x86_64-linux? I'm sure it's a flake that'd probably not appear the next time we trigger it, but it might make sense to harden against this too.

@flokli
Copy link
Contributor

flokli commented Oct 3, 2024

Oh sorry, @afh is the author. 😆

@m1cr0man
Copy link
Contributor

m1cr0man commented Oct 3, 2024

@m1cr0man can you take a look at the failure for x86_64-linux? I'm sure it's a flake that'd probably not appear the next time we trigger it, but it might make sense to harden against this too.

edited and removed comment about http startup error which is actually expected and not related

I can't see why it gave exit code 1, the output is exactly as expected, and I ran it locally and all is good. Perhaps it got oomkilled right at the end or a cleanup step failed?

@afh
Copy link
Member Author

afh commented Oct 3, 2024

The nixosTests.acme on aarch64-darwin Details include the following:

error: a 'aarch64-linux' with features {} is required to build '/nix/store/74sagphiibdwpy2if8wagijl3phz918s-boot.json.drv', but I am a 'aarch64-darwin' with features {benchmark, big-parallel, nixos-test}

@emilazy
Copy link
Member

emilazy commented Oct 3, 2024

It’s a NixOS test, it’s not expected to build on Darwin (without a Linux builder).

@m1cr0man
Copy link
Contributor

m1cr0man commented Oct 8, 2024

@ofborg test acme

@afh
Copy link
Member Author

afh commented Oct 10, 2024

Given that all checks passed, except for the darwin checks, which are expected not to run/pass, what is missing in order to get this reviewed and ideally merged?

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Nothing.

@emilazy emilazy merged commit 5bcc2a2 into NixOS:master Oct 10, 2024
29 of 30 checks passed
@emilazy
Copy link
Member

emilazy commented Oct 10, 2024

(I’m assuming you tested that this works on 32‐bit now?)

@afh afh deleted the update-pebble branch October 10, 2024 03:11
@afh
Copy link
Member Author

afh commented Oct 10, 2024

I did by:

  • creating a temporary package pebble32 = callPackage_i686 ../tools/admin/pebble { }; in pkgs/top-level/all-packages.nix
  • successfully building that using nix build .#pebble32
  • then successfully running ./result/bin/pebble --version
  • all of the above on one of the community builders

Does this suffice as testing it works on 32-bit now, @emilazy?

@emilazy
Copy link
Member

emilazy commented Oct 10, 2024

Yeah, the error looks like a compiler one anyway so checking it builds SGTM :)

Thanks for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants