Skip to content

opencv3,opencv4: disable some unnecessary vendoring on Darwin#256444

Merged
delroth merged 1 commit intoNixOS:masterfrom
delroth:opencv-darwin-unvendor
Sep 27, 2023
Merged

opencv3,opencv4: disable some unnecessary vendoring on Darwin#256444
delroth merged 1 commit intoNixOS:masterfrom
delroth:opencv-darwin-unvendor

Conversation

@delroth
Copy link
Contributor

@delroth delroth commented Sep 21, 2023

Description of changes

See also: https://github.com/opencv/opencv/blob/4.x/CMakeLists.txt#L222 & https://github.com/opencv/opencv/blob/3.4/CMakeLists.txt#L220

Ref #254798

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@delroth
Copy link
Contributor Author

delroth commented Sep 21, 2023

@ofborg build opencv3 opencv4

@delroth delroth mentioned this pull request Sep 21, 2023
Closed
90 tasks
@delroth delroth added 1.severity: security Issues which raise a security issue, or PRs that fix one backport release-23.05 labels Sep 21, 2023
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Sep 21, 2023
@ofborg ofborg bot requested review from basvandijk and mdaiter September 21, 2023 07:24
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Sep 21, 2023
@delroth
Copy link
Contributor Author

delroth commented Sep 21, 2023

Maintainers, I think I'm going to need your help here:

  • opencv4 tests seem to randomly fail on x86_64-linux? It passed in one ofborg invocation, didn't pass in the other, even though there isn't even a diff caused by this PR on x86_64-linux.
  • opencv4-tests fails on Darwin with this PR even though opencv4 itself builds fine, and I don't have access to a Darwin host to debug this further.

In case this wasn't clear in the PR description, this is CVE-2023-4863 remediation related.

@delroth
Copy link
Contributor Author

delroth commented Sep 21, 2023

cc @gbtb who touched the tests last AFAICT and might be able to help too :)

@gbtb
Copy link
Member

gbtb commented Sep 21, 2023 •
edited
Loading

cc @gbtb who touched the tests last AFAICT and might be able to help too :)

Hi. When I've added test support for opencv4 in nixpkgs, I've also noticed that x86-64_darwin tests had not been working properly and chose to ignore them for that platform
Considering flakiness of the tests for x86_64-linux platform, I also had to ignore some categories of tests that failed for various reasons. But then it was stable both locally and on CI. I haven't looked how the tests behave on Hydra since then. Maybe we can ignore offending tests or the whole test suite by removing it from test list here
P.S. FYI, both maintainers listed here haven't been active in a while, and they never responded to me when I worked on my PR :)

risicle
risicle reviewed Sep 21, 2023
View reviewed changes
@delroth delroth force-pushed the opencv-darwin-unvendor branch from 652f79f to c32c0dd Compare September 21, 2023 23:00
@delroth
Copy link
Contributor Author

delroth commented Sep 21, 2023

@ofborg build opencv3 opencv4

@risicle
Copy link
Contributor

risicle commented Sep 22, 2023

nixpkgs-review reveals no new failures, macos 10.15.

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Sep 25, 2023
@delroth
Copy link
Contributor Author

delroth commented Sep 27, 2023

In absence of more reviews from Darwin maintainers, merging. I'm kind of unhappy with the state of this package, the broken tests at HEAD make it really hard to figure out whether new regressions are happening. But given that the alternative is marking this as insecure, I'll take the increased risk chance here - please don't hesitate to revert (but then, fix the vendoring before EOW).

@delroth delroth merged commit 5266b96 into NixOS:master Sep 27, 2023
@github-actions github-actions bot mentioned this pull request Sep 27, 2023
Merged
1 task
@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2023

Successfully created backport PR for release-23.05:

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

Reviewers

@risicle risicle risicle approved these changes

@basvandijk basvandijk Awaiting requested review from basvandijk

@mdaiter mdaiter Awaiting requested review from mdaiter

Assignees

No one assigned

Labels

1.severity: security Issues which raise a security issue, or PRs that fix one 6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@delroth @gbtb @risicle