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

Reduce CoreCLR PAL #76832

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Reduce CoreCLR PAL #76832

merged 1 commit into from
Oct 11, 2022

Conversation

janvorli
Copy link
Member

I have discovered some unused functions in the coreclr PAL. So I have removed it from the PAL and also removed some unused stuf from the coreclr that I have found along the way.
I have also replace the DeleteFile by standard remove function.
The CreatePipe is not used by any code using the PAL except for the PAL tests. But I had to keep it to
preserve CreateProcess tests without refactoring. So I have at least removed it from the pal.h to prevent future usage.
I have also modified couple of PAL tests to get rid of SetEndOfFile API usage that was removed for tests where it made sense to preserve them.

Remove unused functionality from the CoreCLR PAL. Also replace the
`DeleteFile` by standard `remove` function.
@janvorli janvorli added this to the 8.0.0 milestone Oct 10, 2022
@janvorli janvorli self-assigned this Oct 10, 2022
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@jkotas jkotas merged commit f745eb9 into dotnet:main Oct 11, 2022
@jkotas
Copy link
Member

jkotas commented Oct 11, 2022

This introduced a build break in Mono:

2022-10-11T04:38:50.1127810Z   [ 87%] Building CXX object dlls/mscordbi/utilcode/CMakeFiles/utilcode_obj.dir/clrhost_nodependencies.cpp.o
2022-10-11T04:38:50.1205136Z   /__w/1/s/src/mono/dlls/dbgshim/dbgshim.cpp:122:9: error: use of undeclared identifier '_putenv'
2022-10-11T04:38:50.1207257Z           putenv("MONO_ENV_OPTIONS='--debugger-agent=transport=dt_socket,address=127.0.0.1:pid_based,server=n,suspend=y,loglevel=10,timeout=100000'");
2022-10-11T04:38:50.1208250Z           ^
2022-10-11T04:38:50.1341137Z   /__w/1/s/src/mono/dlls/dbgshim/dbgshim.cpp:19:16: note: expanded from macro 'putenv'
2022-10-11T04:38:50.1346925Z   #define putenv _putenv
2022-10-11T04:38:50.1358557Z                  ^

I am going to revert this PR.

jkotas added a commit that referenced this pull request Oct 11, 2022
jkotas added a commit that referenced this pull request Oct 11, 2022
@vargaz
Copy link
Contributor

vargaz commented Oct 11, 2022

Why didn't CI pick up the build breakage ?

@am11
Copy link
Member

am11 commented Oct 11, 2022

@vargaz, the change path evaluation (responsible for triggering CI legs) is missing src/coreclr/pal and src/coreclr/utilcode from:

- subset: mono_excluding_wasm
include:

Since mono desktop depend on coreclr PAL (for corehost), I think we should add those paths in the list which affects mono.

Docs:

#!/usr/bin/env bash
: '
Scenarios:
1. exclude paths are specified
Will include all paths except the ones in the exclude list.
2. include paths are specified
Will only include paths specified in the list.
3. exclude + include:
1st we evaluate changes for all paths except ones in excluded list. If we can not find
any applicable changes like that, then we evaluate changes for included paths
if any of these two finds changes, then a variable will be set to true.
In order to consume this variable in a yaml pipeline, reference it via: $[ dependencies.<JobName>.outputs["<StepName>_<subset>.containschange"] ]
Example:
-difftarget ''HEAD^1'' -subset coreclr -includepaths ''src/libraries/System.Private.CoreLib/*'' -excludepaths ''src/libraries/*+src/installer/*''
This example will include ALL path changes except the ones under src/libraries/*!System.Private.CoreLib/*
'

cc @akoeplinger

@jkotas
Copy link
Member

jkotas commented Oct 11, 2022

the change path evaluation (responsible for triggering CI legs) is missing src/coreclr/pal and src/coreclr/utilcode from:

It is more directories than that: https://github.com/dotnet/runtime/blob/main/src/mono/dlls/mscordbi/CMakeLists.txt#L31-L43

@janvorli
Copy link
Member Author

Ouch, I had no idea that anything in mono uses coreclr PAL.

@AaronRobinsonMSFT
Copy link
Member

Ouch, I had no idea that anything in mono uses coreclr PAL.

Agreed. That seems wrong. Shared code really needs to be moved up to src/native.

@jkotas Was it just putenv or did you find others?

@jkotas
Copy link
Member

jkotas commented Oct 11, 2022

That seems wrong. Shared code really needs to be moved up to src/native.

I think there is larger problem with how the ICorDebug Mono implementation is factored. The first questions to ask would be where it is used and how it is tested and where we want to move it going forward, and only then worry about mechanics.

Was it just putenv or did you find others?

The build break error was just about putenv, but I do not know whether there is more hiding behind it.

@akoeplinger
Copy link
Member

/cc @lambdageek @lateralusX

@janvorli
Copy link
Member Author

I'll try to build Mono and see if anything else breaks. The _putenv is cheap to put back for now as it is just a thin wrapper around EnvironPutenv that is used at few other places.

@janvorli
Copy link
Member Author

I have tried to build mono locally off my branch and it all passed. @jkotas can you point me to the job that has failed with that error?

@akoeplinger
Copy link
Member

@janvorli you need to build the mono.mscordbi subset.

@janvorli
Copy link
Member Author

@akoeplinger I have tried that too before and it succeeded. However, it seems it doesn't build anything native. I've even tried a clean build.

janvorli added a commit that referenced this pull request Oct 12, 2022
janvorli added a commit that referenced this pull request Oct 13, 2022
* Revert "Revert "Reduce CoreCLR PAL (#76832)" (#76860)"

This reverts commit 744fc9b.

* Put back the _putenv since it is used by Mono
@janvorli janvorli deleted the reduce-pal-1 branch October 18, 2022 23:47
@ghost ghost locked as resolved and limited conversation to collaborators Nov 18, 2022
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.

6 participants