Skip to content

Support mvsc on remote build #12

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

Open
wants to merge 5 commits into
base: feature/tipi
Choose a base branch
from

Conversation

Lambourl
Copy link
Contributor

@Lambourl Lambourl commented Apr 6, 2022

This pr is intended to allow remote compiling with msvc

Copy link
Contributor

@pysco68 pysco68 left a comment

Choose a reason for hiding this comment

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

For me this one is clearly non-mergeable in this state. The only part that's clean to me (at first sight) is the change to the macos pkr file.

I'm quite sure the solution you hacked here is not what we should do. I'd rather add some sort of environment aliasing that the cli has to understand (as in ìf there's no my-targert.pkr.js (file|folder) look for a my-target.pkr.js.alias file which can point to another environment and zip up that one)`

@daminetreg what's your take on that one?

@@ -1,4 +1,6 @@
{
"vs-15-2017-win64-cxx17" : "Visual Studio 15 2017 Win64",
"vs-16-2019-win64-cxx17" : "Visual Studio 16 2019"
"vs-16-2019-win64-cxx17" : "Visual Studio 16 2019",
"windows-vs-16-2019-win64-cxx17" : "Visual Studio 16 2019"
Copy link
Contributor

Choose a reason for hiding this comment

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

You just kind-of made just half the work here.

If we're touching that one anyway we should make the due dilligence to list all of the targets that actually need the generator properly set to Visual Studio xx 20yy . Espeacially as you potentially made another addition down below which isn't working (the cxx11 version defined windows-vs-16-2019-win64.cmake namely....)

Comment on lines 1 to 18
# Copyright (c) 2015-2017, Ruslan Baratov
# All rights reserved.

if(DEFINED POLLY_VS_16_2019_WIN64_CXX17_CMAKE_)
return()
else()
set(POLLY_VS_16_2019_WIN64_CXX17_CMAKE_ 1)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/utilities/polly_init.cmake")

polly_init(
"Visual Studio 16 2019 Win64 / C++17"
"Visual Studio 16 2019"
)

include("${CMAKE_CURRENT_LIST_DIR}/utilities/polly_common.cmake")
include("${CMAKE_CURRENT_LIST_DIR}/flags/vs-cxx17.cmake")
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Complete duplicate of vs-16-2019-win64-cxx17.cmake should have used CMake's include() rather than making that copy imho

Comment on lines 1 to 17
# Copyright (c) 2015-2017, Ruslan Baratov
# All rights reserved.

if(DEFINED POLLY_VS_16_2019_WIN64_CMAKE_)
return()
else()
set(POLLY_VS_16_2019_WIN64_CMAKE_ 1)
endif()

include("${CMAKE_CURRENT_LIST_DIR}/utilities/polly_init.cmake")

polly_init(
"Visual Studio 16 2019"
"Visual Studio 16 2019"
)

include("${CMAKE_CURRENT_LIST_DIR}/utilities/polly_common.cmake")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines 96 to 102
{
"type": "powershell",
"inline": [
"Invoke-WebRequest -Uri 'https://bootstrap.pypa.io/get-pip.py' -OutFile 'get-pip.py'",
"tipi run 'python get-pip.py'"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

And 💥 we just introduced a bugfix in a copy which happens to be lacking in the other one.... Which is why this copy is a really bad idea.

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.

3 participants