Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Revert "Revert "Reduce CoreCLR PAL"" #76972

Merged
merged 2 commits into from
Oct 13, 2022

Conversation

janvorli
Copy link
Member

Reverts #76860

@janvorli janvorli added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 12, 2022
@janvorli janvorli self-assigned this Oct 12, 2022
@janvorli
Copy link
Member Author

@am11 this would not work. The PAL maintains its own copy of the environment, so the CreateProcessW would not see the changes added by the OS putenv.

@janvorli janvorli removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) NO-REVIEW Experimental/testing PR, do NOT review it labels Oct 13, 2022
@janvorli janvorli added this to the 8.0.0 milestone Oct 13, 2022
@janvorli janvorli requested a review from jkotas October 13, 2022 00:23
@janvorli
Copy link
Member Author

@jkotas the _putenv turned out to be the only issue, so I've put it back. Can you please take a look at this revert of revert?
Please note I've left a dummy change in one of the files in the eng so that this PR verifies the mono build. I'll remove it before merging the change.

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.

LGTM!

@janvorli janvorli force-pushed the revert-76860-revert-76832-reduce-pal-1 branch from ff18024 to 647cc64 Compare October 13, 2022 08:07
@janvorli janvorli merged commit e6f3aa9 into main Oct 13, 2022
@janvorli janvorli deleted the revert-76860-revert-76832-reduce-pal-1 branch October 13, 2022 11:37
@am11
Copy link
Member

am11 commented Oct 13, 2022

@janvorli, Mono's llvmaot leg is failing with:

  [ 88%] Building CXX object dlls/mscordbi/utilcode/CMakeFiles/utilcode_obj.dir/configuration.cpp.o
  /__w/1/s/src/mono/dlls/mscordbi/cordb-value.cpp:351:44: error: allocating an object of abstract class type 'CordbArrayValue'
          CordbArrayValue* objectValue = new CordbArrayValue(conn, m_pCordbType, m_debuggerId, m_pClass);
                                             ^
  /__w/1/s/src/coreclr/pal/prebuilt/inc/cordebug.h:15647:43: note: unimplemented pure virtual method 'HasBaseIndicies' in 'CordbArrayValue'
          virtual HRESULT STDMETHODCALLTYPE HasBaseIndicies(
                                            ^
  /__w/1/s/src/coreclr/pal/prebuilt/inc/cordebug.h:15650:43: note: unimplemented pure virtual method 'GetBaseIndicies' in 'CordbArrayValue'
          virtual HRESULT STDMETHODCALLTYPE GetBaseIndicies(
                                            ^
  1 error generated.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=51175&view=logs&jobId=cc858e6b-b6f5-5c08-4b62-8cd383bd096a&j=cc858e6b-b6f5-5c08-4b62-8cd383bd096a&t=74861b75-1f67-5a94-490e-d5eb4723f5a5

Looks like dummy change was needed in src/tests as well.

@janvorli
Copy link
Member Author

@am11 this doesn't seem to be related to my changes. I have not removed anything that could result in this error. And yesterday I've built using the same command line that the failing build is using locally and it passed.

@janvorli
Copy link
Member Author

@jkotas seems like it was caused by your #76966.

@am11
Copy link
Member

am11 commented Oct 13, 2022

You are right, it is caused by #76966.

@jkotas
Copy link
Member

jkotas commented Oct 13, 2022

Ugh. #77002

@ghost ghost locked as resolved and limited conversation to collaborators Nov 17, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants