Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Build in Azure Pipelines with cached vcpkg artifacts #114
Build in Azure Pipelines with cached vcpkg artifacts #114
Changes from 15 commits
24f4cc6
56e7e25
21b7433
e041c7e
6106766
3fa17d3
e4b5b69
3adeea0
63d5a19
eb80435
0464ce7
44d4d31
ab4391f
3a5d74e
87912e8
f777520
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems highly invasive. Is it desirable? Could we simply fail if we detect an existing installation of VS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This script is only run on the "hosted" machines which are blown away after each build, so invasive changes are fine/normal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you would always fail on the hosted one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to request the latest Win10 SDK, instead of encoding this version number here that might need to be updated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as far as I am aware.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have a
parameters
block here when the parameters come fromazure-pipelines.yml
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is declaration and default value of accepted params. The actual value is indeed in a-p.yml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rephrasing: do we need to have this declaration here, or can we leave it out so this template will fail when it's not used correctly? It would be a shame if someone accidentally introduced a typo in the parameter names (e.g.,
targetP1atform
) inazure-pipelines.yml
and we consequently buildx64
four times for every PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose if it MUST be here we could give it an invalid value like
potato
to ensure it will fail very loudly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this doing a whole-string or a substring match? If the latter, the
.*
at the front and back are unnecessary.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not positive which it is but I also think it doesn't matter. (And we should merge this as-is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for there to be two newlines here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need boost build explicitly listed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my experience, when running 'vcpkg install boost-math:x86-windows', it failed asking me to install 'boost-build:x86-windows' as well. And doing this failed with same error: 'vcpkg install boost-build:x86-windows boost-math:x86-windows'. I had to run first the installation of boost-build:x86-windows, and then run the installation of boost-math:x86-windows separately to succeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might be a issue worth reporting in vcpkg, but I'm not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observe that this mentions x64 before x86.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file have to be in the root of the repo? Would
azure-devops
be a better location? It's mentioned byazure-devops/run_build.yml
.