Skip to content

Add self contained short flag #19681

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

Merged
merged 3 commits into from
Sep 9, 2021
Merged

Add self contained short flag #19681

merged 3 commits into from
Sep 9, 2021

Conversation

WeihanLi
Copy link
Contributor

@WeihanLi WeihanLi commented Aug 12, 2021

#19576

Add --sc short for --self-contained

@ghost
Copy link

ghost commented Aug 12, 2021

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@richlander
Copy link
Member

Love it. When we merge this, we should also add support an implicit RID at the same time. That will make the feature from good to great.

/cc @agocke

@agocke
Copy link
Member

agocke commented Aug 12, 2021

I think we just need a few tests here. There are some existing tests in GivenDotnetPublishPublishesProject.cs that test --self-contained that could probably be adjusted to test this as well by adding more InlineData clauses

@WeihanLi
Copy link
Contributor Author

I think we just need a few tests here. There are some existing tests in GivenDotnetPublishPublishesProject.cs that test --self-contained that could probably be adjusted to test this as well by adding more InlineData clauses

Great thanks @agocke, would add some test cases

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@agocke
Copy link
Member

agocke commented Aug 16, 2021

@richlander Any more comments?

@agocke
Copy link
Member

agocke commented Aug 16, 2021

Also @wli3 in-case I'm missing anything

@wli3
Copy link

wli3 commented Aug 16, 2021

@KathleenDollard could you double check that? We need to be careful adding new short hands, since we have only 26 letters to use

@wli3
Copy link

wli3 commented Aug 16, 2021

it is good on the technical side

@KathleenDollard
Copy link

Unfortunately, s is source. This is important on the install commands, and it also works on restore.

We avoid having the same abbreviation mean different things, especially for closely related commands. And we work hard to have consistent abbreviations where a particular option is available. build and restore are obviously closely linked commands and it would seem likely consider them sharing more options in the future.

Since build does a restore and supports s, taking this would mean that we could not add --source to pass through to restore. It would also that we could not specify --self-contained for restore (to get the right packages). Unfortunately, c doesn't work and -sc would be confusing for Posix users. We generally avoid two letter abbreviations because of that, but as Will points out, there are only 26 letters and there is value to an abbreviation here.

@AraHaan
Copy link
Member

AraHaan commented Aug 17, 2021

I think there is no choice but to abbreviate this as -sc.

@agocke
Copy link
Member

agocke commented Aug 17, 2021

We could do --sc, which is still short but also fits posix convention

@richlander
Copy link
Member

--sc sounds great to me.

I still want to hold on this change until we add an implicit RID by default.

@AraHaan
Copy link
Member

AraHaan commented Aug 17, 2021

--sc sounds great to me.

I still want to hold on this change until we add an implicit RID by default.

Is there a way to do that based on what it detects on the os and what cpu bitness it detects as well?

@WeihanLi
Copy link
Contributor Author

Thanks, will update to --sc later.

@richlander as for the default implicit RID, I'm going to implement it in another PR #19725, I'm not sure about the changes, maybe @agocke could help?

@WeihanLi WeihanLi changed the title Add '-s' for self contained Add self contained short flag Aug 17, 2021
@WeihanLi WeihanLi requested a review from agocke August 17, 2021 06:10
@wli3
Copy link

wli3 commented Aug 17, 2021

FYI @KathleenDollard is not available today.

@marcpopMSFT marcpopMSFT changed the base branch from main to release/6.0.1xx September 8, 2021 21:26
@marcpopMSFT marcpopMSFT disabled auto-merge September 8, 2021 22:59
@marcpopMSFT
Copy link
Member

Got @KathleenDollard's signoff offline. @wli3 , anything else before we approve and merge?

@AraHaan
Copy link
Member

AraHaan commented Sep 9, 2021

Is this ready to merge yet?

@wli3 wli3 merged commit 505ad13 into dotnet:release/6.0.1xx Sep 9, 2021
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.

7 participants