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

Remove always-defined symbols #9044

Merged
merged 19 commits into from Jan 23, 2017

Conversation

Projects
None yet
5 participants
@danmosemsft
Member

danmosemsft commented Jan 23, 2017

These are always defined in the CoreCLR build, and now we no longer want to preserve mergeability with desktop, we can remove them.

Fixes #8883

Verification in progress: diff decompilation for each architecture.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jan 23, 2017

Member

Verified by building before and after and decompiling then diffing the code across Windows debug, Windows x86, Linux debug, Linux release. The only diff in each case is the dead string I removed.

Member

danmosemsft commented Jan 23, 2017

Verified by building before and after and decompiling then diffing the code across Windows debug, Windows x86, Linux debug, Linux release. The only diff in each case is the dead string I removed.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jan 23, 2017

Member

@jkotas could you or someone review?

Member

danmosemsft commented Jan 23, 2017

@jkotas could you or someone review?

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jan 23, 2017

Member

FeatureAppdomainResourceMonitoring should be deleted & disabled instead.

Member

jkotas commented Jan 23, 2017

FeatureAppdomainResourceMonitoring should be deleted & disabled instead.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jan 23, 2017

Member

LGTM. Thanks for getting this started!

BTW: I have opened the following issues related to the NetStandard2.0 work as I was reviewing this:

#9045
#9047

cc @gkhanna79 @rahku

Member

jkotas commented Jan 23, 2017

LGTM. Thanks for getting this started!

BTW: I have opened the following issues related to the NetStandard2.0 work as I was reviewing this:

#9045
#9047

cc @gkhanna79 @rahku

@hughbe

This comment has been minimized.

Show comment
Hide comment
@hughbe

hughbe Jan 23, 2017

Contributor

Can you clarify why we've abandoned mergeability with Desktop? Thanks

Contributor

hughbe commented Jan 23, 2017

Can you clarify why we've abandoned mergeability with Desktop? Thanks

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jan 23, 2017

Member

It is not completely abandoned, e.g. it is still maintained for some parts like JIT or GC. The corelib is on the port the selected fixes plan - same as corefx, for the same reasons as corefx - the .NET Core corelib is too different from the desktop one.

Member

jkotas commented Jan 23, 2017

It is not completely abandoned, e.g. it is still maintained for some parts like JIT or GC. The corelib is on the port the selected fixes plan - same as corefx, for the same reasons as corefx - the .NET Core corelib is too different from the desktop one.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jan 23, 2017

Member

OK, removed and disabled FEATURE_APPDOMAIN_RESOURCE_MONITORING. This is the only change affecting the final binary (save removing a string)

Member

danmosemsft commented Jan 23, 2017

OK, removed and disabled FEATURE_APPDOMAIN_RESOURCE_MONITORING. This is the only change affecting the final binary (save removing a string)

@danmosemsft danmosemsft merged commit 4c4f4be into master Jan 23, 2017

13 checks passed

CentOS7.1 x64 Debug Build and Test Build finished.
Details
FreeBSD x64 Checked Build Build finished.
Details
Linux ARM Emulator Cross Debug Build Build finished.
Details
Linux ARM Emulator Cross Release Build Build finished.
Details
OSX x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Checked Build and Test Build finished.
Details
Ubuntu x64 Formatting Build finished.
Details
Windows_NT arm Cross Debug Build Build finished.
Details
Windows_NT arm Cross Release Build Build finished.
Details
Windows_NT x64 Debug Build and Test Build finished.
Details
Windows_NT x64 Formatting Build finished.
Details
Windows_NT x64 Release Priority 1 Build and Test Build finished.
Details
Windows_NT x86 Checked Build and Test Build finished.
Details
@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jan 23, 2017

Member

@jkotas merged, but just a note to confirm that you meant me to remove FEATURE_APPDOMAIN_RESOURCE_MONITORING (it was previously enabled)

Member

danmosemsft commented Jan 23, 2017

@jkotas merged, but just a note to confirm that you meant me to remove FEATURE_APPDOMAIN_RESOURCE_MONITORING (it was previously enabled)

@danmosemsft danmosemsft deleted the redundant_defs branch Jan 23, 2017

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Jan 23, 2017

Member

@danmosemsft Yes, looks good.

Member

jkotas commented Jan 23, 2017

@danmosemsft Yes, looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment