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

[qhull] Disable tools install on iOS #27379

Closed
wants to merge 1 commit into from
Closed

Conversation

DDoS
Copy link
Contributor

@DDoS DDoS commented Oct 21, 2022

CMake doesn't generate usable tool executables when cross-compiling to iOS, and the install step is broken due to invalid paths in the targets file.

  • What does your PR fix?

    Skip installing the iOS tools, they don't run and there's no real use for them anyway.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

@dg0yt
Copy link
Contributor

dg0yt commented Oct 21, 2022

There is #27260.

@DDoS
Copy link
Contributor Author

DDoS commented Oct 21, 2022

There is #27260.

Yeah that's a cleaner solution, but it's waiting on upstream, and who knows when the next Qhull version will be released.
I think this issue is better for patching the iOS build while we wait for an upstream fix.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 21, 2022

TBH I don't why the other PR is requested to wait. Of couse that patch could be minimized, without the formatting changes.

@BillyONeal
Copy link
Member

TBH I don't why the other PR is requested to wait. Of couse that patch could be minimized, without the formatting changes.

We aren't qhull, we are a packaging solution, so we don't want to speak for them. Some patch principles:

  • We prefer an official upstream release with the changes.
  • We want to avoid patches that:
    • upstream would disagree with
    • cause vulnerabilities or crashes
    • we are incapable of maintaining across upstream version updates
    • are large enough to cause license entanglement with the vcpkg repository itself

That PR adds a new deployment scheme / knob, so we believe upstream needs an opportunity to weigh in. It's a build system feature and not a product feature, we're willing to take the patch if they don't respond.

@JonLiu1993 JonLiu1993 added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist depends:upstream-changes Waiting on a change to the upstream project labels Oct 24, 2022
@DDoS
Copy link
Contributor Author

DDoS commented Oct 25, 2022

@BillyONeal Considering that #27386 says

We will skip this waiting period if we have high confidence that the change is correct. Examples of high confidence patches include, but are not limited to:
[...]

  • Disabling irrelevant-in-vcpkg components of the build such as tests or examples.

Does this PR actually need to wait on upstream? The tools are irrelevant on iOS. There is #27260 that can supersede this if upstream agrees to take in changes to disable the tools build. This is only avoids installing them to avoid a bug at the last moment possible.

@BillyONeal
Copy link
Member

There is #27260.

Yeah that's a cleaner solution, but it's waiting on upstream, and who knows when the next Qhull version will be released. I think this issue is better for patching the iOS build while we wait for an upstream fix.

That has been merged. Do you still want this change?

@DDoS
Copy link
Contributor Author

DDoS commented Nov 18, 2022

There is #27260.

Yeah that's a cleaner solution, but it's waiting on upstream, and who knows when the next Qhull version will be released. I think this issue is better for patching the iOS build while we wait for an upstream fix.

That has been merged. Do you still want this change?

No real need for this PR with the other one merged.

@DDoS DDoS closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist depends:upstream-changes Waiting on a change to the upstream project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants