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

Don't let unsupported ports pull in dependencies #846

Closed
wants to merge 3 commits into from

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Dec 30, 2022

When a port mapnik is unsupported on x64-uwp, its dependency on boost-regex[icu] must not take effect during vcpkg ci --triplet=x64-uwp because it will make boost-regex unbuildable on uwp as long as icu is unsupported on this triplet.

Background: https://github.com/microsoft/vcpkg/pull/28619/files#r1059405866

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 30, 2022

This PR should remove a number of blocking "cascades", so the modified tool must be run in the vcpkg repo CI to identify build failures in then-unblocked ports.

@dg0yt dg0yt marked this pull request as ready for review December 30, 2022 19:03
@autoantwort
Copy link
Contributor

With #802 we get something similar (all features of boost-regex gets tested and boost-regex[icu] only gets build when needed)

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 1, 2023

With #802 we get something similar (all features of boost-regex gets tested and boost-regex[icu] only gets build when needed)

Possible, but I miss details of what that PR will do and how it is to be used.

@autoantwort
Copy link
Contributor

We could go a step further:

When a port mapnik is unsupported on x64-uwp, its dependency on boost-regex[icu] must not take effect during vcpkg ci --triplet=x64-uwp because it will make boost-regex unbuildable on uwp as long as icu is unsupported on this triplet.

In this case we also should skip mapnik (so when a port pulls in unsupported ports)

Copy link
Contributor

@autoantwort autoantwort left a comment

Choose a reason for hiding this comment

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

Should the skips be shown in the output?

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 12, 2023

In this case we also should skip mapnik (so when a port pulls in unsupported ports)

I don't understand this point. The skipping of building mapnik by means of supports is already in vcpkg tool.

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 12, 2023

Should the skips be shown in the output?

Which skips? Do we need CI output for unsupported ports?

@autoantwort
Copy link
Contributor

Do we need CI output for unsupported ports?

Not necessary, but maybe it helps to understand what is happening.

@autoantwort
Copy link
Contributor

Just for note: This does not fix microsoft/vcpkg#23490, for explanation see #856 (comment)

@dg0yt
Copy link
Contributor Author

dg0yt commented Jan 12, 2023

Just for note: This does not fix microsoft/vcpkg#23490, for explanation see #856 (comment)

Actually it does fix it: microsoft/vcpkg#23490 doesn't discuss port qt, it is only about qtwebengine and its host effect. This is the CI command bug fixed by my PR: unsupported ports pull in dependencies.

Port qt is a different story. If if claims to support x86 but unconditionally depends on qtwebengine, then it is a bug in this port (qt). I agree it would be good to provide better diagnostics for this case. But this is a different scope. Maybe no longer just commands.ci.cpp. Maybe close to implictly requiring pass from any ports which qualifies through supports.

@autoantwort
Copy link
Contributor

Maybe close to implictly requiring pass from any ports which qualifies through supports.

I have done this in #802 by requiring = cascade

@autoantwort
Copy link
Contributor

If if claims to support x86 but unconditionally depends on qtwebengine

That also causes qtwebview to not build in ci (only with my PR) because of the wrong platform statement

@autoantwort
Copy link
Contributor

Since #856 was merged I think this can be closed :)

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 16, 2023

Only when the tool is released and activated in vcpkg ci.
Actually I'm a little bit frustrated of how this contributions was handled.

@BillyONeal
Copy link
Member

Only when the tool is released and activated in vcpkg ci.

Turned on now that microsoft/vcpkg#29664 is merged :)

Actually I'm a little bit frustrated of how this contributions was handled.

I'm confused, considering the change we merged from @autoantwort seems to be basically the same as your change plus extra stuff?

@Neumann-A
Copy link
Contributor

I'm confused, considering the change we merged from @autoantwort seems to be basically the same as your change plus extra stuff?

II think the point here is that this PR existed before the other one.

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 26, 2023

Actually I'm a little bit frustrated of how this contribution was handled.

I'm confused, considering the change we merged from @autoantwort seems to be basically the same as your change plus extra stuff?

This PR was the initial work. The only official feedback was approval on Jan 11.
#856 came Jan 12, basically reusing part of my original work with different filenames but no git trace.
I did have a few more changes which overlapped with #856 but which I didn't want to mix with this PR, in particular after approval.
And after all this was just tool part. I knew that this would have effects on the ports CI. #846 (comment) was meant to make this clear: I wanted a full CI run with this PR in order identify necessary changes to the ports repo before these changes can be released as part of the tool.

@dg0yt dg0yt closed this Feb 26, 2023
@BillyONeal
Copy link
Member

This PR was the initial work. The only official feedback was approval on Jan 11.

Yes, this is great direction!

#856 came Jan 12, basically reusing part of my original work with different filenames but no git trace.

I viewed that change as effectively the same with a few PR comments fixed, and since it was also ready to go I just merged it there rather than try to get those changes reincorporated here. Moreover, the changes in the other PR were more easy for me to see as "correct" in that they just reuse the existing planner, while I needed additional help from folks who understand the dependency stuff better to totally land this.

There would have not been much point in trying to preserve a git trace: that doesn't survive the squash anyway.

I agree though that you should have gotten called out / more credit for the original author of the changes than you got. I just updated the release notes for that version of the tool to call out that this change was originally your idea.

I did have a few more changes which overlapped with #856 but which I didn't want to mix with this PR, in particular after approval.

Ah, I'm sorry. I didn't intend to convey "don't change this" with approval.

And after all this was just tool part. I knew that this would have effects on the ports CI. #846 (comment) was meant to make this clear: I wanted a full CI run with this PR in order identify necessary changes to the ports repo before these changes can be released as part of the tool.

I see, in this case I signed up to deal with that fallout and I didn't think it would be worth bothering you about it.

@dg0yt
Copy link
Contributor Author

dg0yt commented Feb 28, 2023

There would have not been much point in trying to preserve a git trace: that doesn't survive the squash anyway.

It is not so bad. Example:
microsoft/vcpkg@82e0390

@autoantwort
Copy link
Contributor

I would have had to add you manually anyway since I did not reused your code (I wrote my PR from scratch). I only adapted the --dry-run changes. But it would have been fair to add you as co author since creating a PR was your idea (and the --dry-run changes).

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

Successfully merging this pull request may close these issues.

4 participants