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

Support MSBuild properties on the CLI #152

Merged
merged 9 commits into from
Nov 13, 2023
Merged

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Nov 9, 2023

@baronfel Is that roughly what you had in mind in #84 ?

With it we can do things like:

-p Configuration=Release -p DefineConstants="MYDEF" ...

@baronfel
Copy link
Collaborator

baronfel commented Nov 9, 2023

Yep! This would do the base-level support - though MSBuild properties can be passed via -p in a number of different formats. You can see the tests we have in the SDK for our own parsing of the various formats here. I think simple key=value pairs are the most common, but automated tools especially love to use the more complex formats.

@dawedawe
Copy link
Contributor Author

Thanks for the pointer. These should all work now with -p or -p: or --property.

@dawedawe
Copy link
Contributor Author

If the following mapping to MSBuild properties is correct, I think this PR is ready for review (not sure about OS):
-r maps to RuntimeIdentifier
-a maps to Platform
--os maps to OS

@dawedawe dawedawe marked this pull request as ready for review November 10, 2023 17:32
@baronfel
Copy link
Collaborator

--os and --arch map to different components of a RuntimeIdentifier. Take linux-x64 for example - the linux portion is the OS, and the x64 portion is the arch. This isn't a hard-and-fast rule, however - the Runtime Identifier linux-musl-x64 has an os of linux-musl and an arch of x64.

The dotnet CLI errors if arch/os are provided at the same time as -r, and it 'fills in' any missing pieces of the final RuntimeIdentifier from arch/os with the arch/os of the current SDK. So a user specifying --arch arm64 on a linux device would end up passing -p RuntimeIdentifier=linux-arm64 to the build.

@dawedawe
Copy link
Contributor Author

Ah okay sorry, I had some bigger misunderstanding then.
So I need to find a way how to query programatically the correct string values for os and arch then...

@baronfel
Copy link
Collaborator

The 'correct' way to get the RID for the currently running SDK is here - but it is very, very gross.

@dawedawe
Copy link
Contributor Author

So System.Runtime.InteropServices.RuntimeInformation.RuntimeIdentifier can be wrong in some cases?

@baronfel
Copy link
Collaborator

oh, great idea - that should be fine for your needs. I think the SDK has some layering problem where it can't necessarily rely on that.

@dawedawe
Copy link
Contributor Author

Ok, thanks a lot for all your help here!

configuration
|> Option.map (fun c -> List.append props [ "Configuration", c ])
|> Option.defaultValue props
let properties = getProperties results
Copy link
Contributor

Choose a reason for hiding this comment

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

Can properties be combined with --fsc-args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect they can be combined but I'll test next week.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thinking is that they cannot. Properties would influence the project cracking I assume and we skip that entirely when using --fsc-args.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed, while we could shoehorn some parts from the properties into the fsc-args, it would be against the idea of fsc-args, so let's disallow that.

@dawedawe
Copy link
Contributor Author

With the RuntimeIdentifier being constructed with --arch and --os if given, I hope this is ready now.

Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Looks good, ship it!

@dawedawe dawedawe merged commit 989eef8 into ionide:master Nov 13, 2023
2 checks passed
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