Skip to content

[pbc] Correct non Windows build#9281

Merged
vicroms merged 2 commits intomicrosoft:masterfrom
decent-dcore:master
Jan 22, 2020
Merged

[pbc] Correct non Windows build#9281
vicroms merged 2 commits intomicrosoft:masterfrom
decent-dcore:master

Conversation

@decent-dcore
Copy link
Contributor

@decent-dcore decent-dcore commented Dec 11, 2019 •
edited
Loading

  • 'mpir' build dependency is Windows specific
  • cleaned up deprecated stuff

@msftclas
Copy link

msftclas commented Dec 11, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@decent-dcore decent-dcore marked this pull request as ready for review December 12, 2019 08:05
@PhoebeHui PhoebeHui self-requested a review December 13, 2019 01:42
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 13, 2019

@decent-dcore, thanks for the PR!

Could you include below information?

  1. What issue does this PR fix? Please open an issue so others may replicate this problem.
  2. Please update the baseline CI file under line https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt#L1269

PhoebeHui
PhoebeHui reviewed Dec 13, 2019
View reviewed changes
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 13, 2019

/azp run

@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 13, 2019

@decent-dcore, it looks the CI failed on uwp is expected. Could you update the baseline CI file https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt?

pbc:arm-uwp=fail
pbc:x64-uwp=fail

@decent-dcore
Copy link
Contributor Author

decent-dcore commented Dec 13, 2019

@decent-dcore, it looks the CI failed on uwp is expected. Could you update the baseline CI file https://github.com/microsoft/vcpkg/blob/master/scripts/ci.baseline.txt?

pbc:arm-uwp=fail
pbc:x64-uwp=fail

Done, UWP is not supported.

@PhoebeHui PhoebeHui self-assigned this Dec 17, 2019
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 17, 2019

@decent-dcore, an issue required for this changes, could you file an issue?

PhoebeHui
PhoebeHui approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

The changes looks good for me.

@decent-dcore
Copy link
Contributor Author

decent-dcore commented Dec 17, 2019

@PhoebeHui Why an issue is required? There are 1000+ unresolved ones, I do not think that one more makes some difference.

@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 17, 2019

@decent-dcore, we required an issue since it's used for tracking the situation that others may encounter. eg, 'mpir' build dependency is Windows specific, however it should be port bug issue since it didn't rely on it on windows before, other users may want to know more background about it, so an issue for it may be more clear.

@decent-dcore
Copy link
Contributor Author

decent-dcore commented Dec 17, 2019

@PhoebeHui Ok, here's the issue #9348

@PhoebeHui PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 17, 2019
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Dec 17, 2019

Thanks!

@dan-shaw
Copy link
Contributor

dan-shaw commented Dec 19, 2019

LGTM, will merge after MacOS CI pipeline is back up

@dan-shaw dan-shaw assigned dan-shaw and unassigned PhoebeHui Dec 19, 2019
dan-shaw
dan-shaw reviewed Dec 20, 2019
View reviewed changes
@dan-shaw dan-shaw removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jan 6, 2020
@PhoebeHui PhoebeHui mentioned this pull request Jan 13, 2020
Merged
15 tasks
@PhoebeHui PhoebeHui self-assigned this Jan 14, 2020
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Jan 15, 2020

@decent-dcore , could you please resolve the conflicting files?
It seems that the static library check was added by the author when this port supported in vcpkg, commit is 77163c57.

@rfric
Copy link
Contributor

rfric commented Jan 16, 2020

@PhoebeHui conflict resolved

rfric
rfric approved these changes Jan 16, 2020
View reviewed changes
@PhoebeHui
Copy link
Contributor

PhoebeHui commented Jan 17, 2020

@rfric , thanks for your updates!

The failures on OSX are not related to this changes. It should not block to merge this PR.

@PhoebeHui PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jan 20, 2020
@vicroms vicroms merged commit 3aa5979 into microsoft:master Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PhoebeHui PhoebeHui PhoebeHui approved these changes

+2 more reviewers

@dan-shaw dan-shaw dan-shaw approved these changes

@rfric rfric rfric approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@PhoebeHui PhoebeHui

@dan-shaw dan-shaw

Labels

info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@decent-dcore @msftclas @PhoebeHui @dan-shaw @rfric @vicroms