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.

Sign up for GitHub

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

Jump to bottom

Fixes msvc2015 detection when only vs2019 are installed. #1955

Merged
merged 1 commit into from
Jul 12, 2021
Merged

Fixes msvc2015 detection when only vs2019 are installed. #1955

merged 1 commit into from
Jul 12, 2021

Conversation

lygstate
Copy link
Contributor

@lygstate lygstate commented Jun 26, 2021 •
edited
Loading

Signed-off-by: Yonggang Luo [email protected]

Fixed:
#1905

@andreeis
Copy link
Contributor

andreeis commented Jul 8, 2021 •
edited
Loading

@lygstate, we got to review this for 1.8. I merged with the latest changes in develop and I am testing.
Even your first commit did not include util.envSet and util.envGetValue in util.ts. First I thought that I merged something wrong.
I did some changes on my side to unblock testing but if you had anything special in mind for envSet and engGetValue please push an update.

@andreeis
Copy link
Contributor

andreeis commented Jul 8, 2021 •
edited
Loading

Also fixes the output encoding.

@lygstate, the output encoding fix is in util.parseVersion? Do we have a GitHub issue created for that?

andreeis
andreeis reviewed Jul 9, 2021
View reviewed changes
src/kit.ts Outdated Show resolved Hide resolved
@lygstate
Copy link
Contributor Author

lygstate commented Jul 9, 2021

Also fixes the output encoding.

@lygstate, the output encoding fix is in util.parseVersion? Do we have a GitHub issue created for that?

I split the fix of encoding problem in separate PR #1985

@andreeis
Copy link
Contributor

andreeis commented Jul 9, 2021

@lygstate, if you want you can push an update. I tested this and I sign off with 2 comments:

  • revert the vcvars file name change as it was before this PR, for arm32/arm64
  • figure out what you wanted with util.envGetValue and util.envSet. For now, when testing, I simply used var.get and var.set.
    If you won't have the chance, I'll push an update or start a new PR from my own branch (derived from yours) later tomorrow.

@andreeis
Copy link
Contributor

andreeis commented Jul 9, 2021

@lygstate, I noticed that you mentioned that this PR depends on #1763. We may not be able to test thoroughly the env var consistency PR before we release 1.8. It was a large PR with interesting scenarios to cover. When I tested this msvc PR it worked fine. Let me know if I overlooked something. Do you think this msvc PR can work without the env var consistency one?

Another question, can you describe the encoding problem? Just to make sure I properly test that one too and I don't miss something. What was happening.

@bobbrow bobbrow added this to the 1.8.0 milestone Jul 9, 2021
@andreeis
Copy link
Contributor

andreeis commented Jul 9, 2021

@lygstate, for an example of what I needed to change in order to have successful testing see branch dev/andris/cmake-tools/fromPR1955.

This was referenced Jul 9, 2021
Merged
Closed
@lygstate
Fixes msvc2015 detection when only vs2019 are installed.
3b73756 
Currently msvc2015 detection will detected without proper Windows SDK path
so fixes it by patching the Windows SDK path
Also by the help of Andreea Isac, fix arm/aarch64 detection

Signed-off-by: Yonggang Luo <[email protected]>
Signed-off-by: Andreea Isac <[email protected]>
@andreeis andreeis merged commit 1aa0fb6 into microsoft:develop Jul 12, 2021
@lygstate lygstate deleted the msvc2015-detect branch July 14, 2021 08:53
@lygstate lygstate mentioned this pull request Jul 27, 2021
Merged
@bobbrow
Copy link
Member

bobbrow commented Aug 17, 2021

@lygstate Is it necessary to add 'Platform' to the list of MSVC variables to make your PR succeed? We are experiencing a regression in building some of our projects and this blocks the release of 1.8.0. @elahehrashedi has PR #2056 out where she removes this from the list and it fixes our problem. Do you see any problem with us removing it?

@github-actions github-actions bot locked and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@andreeis andreeis andreeis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@lygstate @andreeis @bobbrow