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

Allow to select specific version of msc in command line. #2076

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

Jarod42
Copy link
Contributor

@Jarod42 Jarod42 commented May 8, 2023

What does this PR do?

Allow to select specific version of msc in command line

How does this PR change Premake's behavior?

No changes, except option "cc"

Anything else we should know?

No

Did you check all the boxes?

  • Focus on a single fix or feature; remove any unrelated formatting or code changes
  • Add unit tests showing fix or feature works; all tests pass
  • Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
  • Follow our coding conventions
  • Minimize the number of commits
  • Align documentation to your changes

You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!

@redorav
Copy link
Collaborator

redorav commented Jul 1, 2023

I have a question, how is this different to choosing the compiler via the toolset keyword?

@Jarod42
Copy link
Contributor Author

Jarod42 commented Jul 1, 2023

@redorav:
It is from the command line,
else I might add extra option in premake5.lua to mimic existing --cc=..
(I use same premake5.lua script but with different generators/toolsets).

@redorav
Copy link
Collaborator

redorav commented Jul 1, 2023

Ah I see, I haven't configured projects much from the command line. How would you choose the clang or gcc version in a similar fashion? Or would that be unrelated?

@Jarod42
Copy link
Contributor Author

Jarod42 commented Jul 2, 2023

Never select gcc/clang version from premake (pretty sure it is not possible from command line).
except for supported standard, I don't know if there are flags different depending on version for gcc/clang.
There is for msvc.

@redorav
Copy link
Collaborator

redorav commented Jul 2, 2023

I see, looks good to me then. How does the process normally work, write the review? I'd like to be a bit more active with Premake and get to know the normal workflows you guys are using

@Jarod42
Copy link
Contributor Author

Jarod42 commented Jul 3, 2023

Simple contributor here :-)
So creating PRs, answer questions and do some reviews.
but there are owner/dev of premake which do the final approve (if any).

@nickclark2016
Copy link
Member

I'm currently abroad on vacation. Looks like I have a few reviews and issues to triage when I get back if someone else hasn't. Should be able to look around the 15th or so.

@nickclark2016
Copy link
Member

Quick question: What was the previous behavior of just putting "msc" as an argument? Is there behavior there that we should preserve rather than remove?

Also, are there existing unit tests for CLI arguments? Or is this an area of test deficiency?

@Jarod42
Copy link
Contributor Author

Jarod42 commented Jul 3, 2023

What was the previous behavior of just putting "msc" as an argument?

Putting simply msc, we don't have version so all version checks assume the oldest. so problematic for some flags as externalincludedirs.

Is there behavior there that we should preserve rather than remove?

msc was added recently by me.
I don't thing keeping simple msc is useful.

Also, are there existing unit tests for CLI arguments? Or is this an area of test deficiency?

I don't think there is UTs for CLI arguments except maybe to check that --options=value feeds _OPTION table.

Copy link
Collaborator

@redorav redorav left a comment

Choose a reason for hiding this comment

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

If you're happy with the version selection from the command as state in the comments I'd say let's get it merged

@redorav redorav merged commit 78ce281 into premake:master Jul 6, 2023
@Jarod42 Jarod42 deleted the cc_msc-vXX branch July 7, 2023 12:42
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