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

Make browser peer for windows_nt and unix #37944

Merged
merged 50 commits into from
Jul 8, 2020
Merged

Make browser peer for windows_nt and unix #37944

merged 50 commits into from
Jul 8, 2020

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Jun 16, 2020

Fixes #38559

Contributes to #37439

@ghost
Copy link

ghost commented Jun 16, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@Anipik Anipik changed the title [Donot Review]make browser peer for windows_nt and unix Make browser peer for windows_nt and unix Jun 17, 2020
@Anipik
Copy link
Contributor Author

Anipik commented Jun 18, 2020

cc @ViktorHofer @safern

@Anipik
Copy link
Contributor Author

Anipik commented Jun 18, 2020

@ericstj @marek-safar can you review this one ?

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

For some of these cases we should see if we can represent netcoreapp-Browser;netcoreapp-Unix;netcoreapp-Windows_NT as netcoreapp;netcoreapp-Windows_NT instead (especially in the cases where there is no DLLImport).

There seem to be a few cases where cross-compilation is resulting because of our config system, and not due to any real ifdefs/conditional compilation (EG: anything project referencing System.Private.Uri/System.Runtime, System.Private.Xml). This is similar to the case we relaxed for System.Private.CoreLib. I wonder if we should relax it in these cases, use the vertical-defined configuration of the project reference, and remove the cross-compilation.

@ViktorHofer
Copy link
Member

I'm not sure if all the test --> src configurations are right but we can otherwise fix them in a follow-up PR. Spot checked. Thanks

@Anipik
Copy link
Contributor Author

Anipik commented Jul 8, 2020

@akoeplinger can you review this one ?
i can address the caching change in a follow up pr as it might break something else. I will like to get this one in to avoid merge conflicts.

Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@akoeplinger
Copy link
Member

The pending runtime (CoreCLR GCC Product Build Linux x64 checked) is actually green on AzDO: https://dev.azure.com/dnceng/public/_build/results?buildId=720121&view=results

@akoeplinger akoeplinger merged commit 5ce04cc into dotnet:master Jul 8, 2020
akoeplinger added a commit to akoeplinger/runtime that referenced this pull request Jul 8, 2020
Saw these missing while reviewing dotnet#37944.
As far as I can see this is only used when building inside VS but still good to add for consistency.
akoeplinger added a commit that referenced this pull request Jul 8, 2020
Saw these missing while reviewing #37944.
As far as I can see this is only used when building inside VS but still good to add for consistency.
public static class ODBC32
{
[System.Runtime.CompilerServices.TypeForwardedFrom("System.Data, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
public enum RETCODE : int
Copy link
Member

Choose a reason for hiding this comment

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

What is this about? This needs a better comment at least.

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 added the comment in #38968. there is some more info on decision here #37944 (comment)

@Anipik
Copy link
Contributor Author

Anipik commented Jul 8, 2020

There are going to be follow up prs for this work, i will address the feedback in the next one

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Browser Implementation For various assemblies