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

Reapply "openobserve: 0.7.2 -> 0.8.1" #289751

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Feb 18, 2024

Description of changes

This reapplies #289522, which originally broke the pkgs/by-name check on master,
so it was reverted in #289655.

This reapplies the original commits and makes sure that the pkgs/by-name check works by moving it out of pkgs/by-name.

pkgs/by-name is limited to pkgs.callPackage. See here for the detailed list of checks. It using pkgs.callPackage is a hard requirement, though it can easily be confusing with the "soft" ratchet checks, which wouldn't break master.

I fully admit that the errors for pkgs/by-name should be better to avoid such situations. I'll improve the error reporting for the pkgs/by-name check next week to improve this, see NixOS/nixpkgs-vet#6 for updates. It's really not great that this has delayed a security fix.

Ping @risicle @happysalada @adisbladis @mweinelt

Things done

  • Checked that the package instantiates.

Add a 👍 reaction to pull requests you find important.

@happysalada
Copy link
Contributor

happysalada commented Feb 18, 2024

I was testing a mostly similar one here #289526
it doesn't use darwin.callPackage and uses the normal callPackage
I'm testing right now that this works on x86_64-darwin.
It also doesn't apply the patch as I couldn't figure out what the patch was for actually.
I'm indifferent to which one gets merged.

@infinisil
Copy link
Member Author

@happysalada Oh yeah that one would be more ideal! It shouldn't require explicit testing, as long as the derivation hashes match

@happysalada
Copy link
Contributor

alight, I'll wait for ofborg to be happy and for my local build to finish before merging.

Comment on lines +11470 to +11472
openobserve = darwin.apple_sdk_11_0.callPackage ../servers/monitoring/openobserve {
apple_sdk = darwin.apple_sdk_11_0;
};
Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

Suggested change
openobserve = darwin.apple_sdk_11_0.callPackage ../servers/monitoring/openobserve {
apple_sdk = darwin.apple_sdk_11_0;
};
openobserve = callPackage ../servers/monitoring/openobserve {
stdenv = if stdenv.isDarwin then overrideSDK stdenv "11.0" else stdenv;
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely. It looks like there's also a separate rustPlatform attribute provided by the darwin.apple_sdk_11_0.callPackage.

@LeSuisse LeSuisse added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Feb 18, 2024
This reverts commit ac637ef.

The original PR (NixOS#289522) broke the
`pkgs/by-name` check on master,
so it was reverted in NixOS#289655.

This reapplies the original commits and makes sure that the
`pkgs/by-name` check works by moving it out of `pkgs/by-name`.
@infinisil
Copy link
Member Author

@happysalada @wegank I'll just merge this one for now, since the original PR was already tested and merged. Any refactorings can be done with a separate PR.

@infinisil infinisil merged commit 224c493 into NixOS:master Feb 19, 2024
19 checks passed
@infinisil infinisil deleted the reapply-openobserve-update branch February 19, 2024 16:00
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