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

Revert "Set AppContext.BaseDirectory and the lookup path for AssemblyDirectory to the directory containing the NativeAOT module" #113211

Merged
merged 1 commit into from
Mar 6, 2025

Conversation

MichalStrehovsky
Copy link
Member

Reverts #112457

@Copilot Copilot bot review requested due to automatic review settings March 6, 2025 10:52
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR reverts the changes made in #112457 to restore the previous behavior for determining the application base directory and module path.

  • Replaces the NativeAOT-specific GetRuntimeModulePath with Environment.ProcessPath in AppContext.AnyOS.cs.
  • Updates the return types in DeveloperExperience.cs and RuntimeAugments.cs to be non-nullable, and removes the GetRuntimeModulePath helper from AppContext.NativeAot.cs.
  • Removes the SharedLibraryDependencyLoading test, which may affect shared library dependency loading coverage.

Reviewed Changes

File Description
src/libraries/System.Private.CoreLib/src/System/AppContext.AnyOS.cs Reverts the use of GetRuntimeModulePath to Environment.ProcessPath for NativeAOT.
src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/DeveloperExperience/DeveloperExperience.cs Adjusts the type of moduleFullFileName to non-nullable string, requiring additional handling of empty values.
src/coreclr/nativeaot/System.Private.CoreLib/src/System/AppContext.NativeAot.cs Removes an unused NativeAOT-specific helper method.
src/coreclr/nativeaot/System.Private.CoreLib/src/Internal/Runtime/Augments/RuntimeAugments.cs Changes the return type of TryGetFullPathToApplicationModule from nullable to non-nullable, potentially introducing null-safety issues.
src/tests/nativeaot/SmokeTests/SharedLibraryDependencyLoading/SharedLibraryDependencyLoading.cs Deletes the test file related to shared library dependency loading.

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/tests/nativeaot/SmokeTests/SharedLibraryDependencyLoading/SharedLibraryDependencyLoading.cs:1

  • The removal of this test file could reduce coverage for shared library dependency loading. Confirm that this change is intentional and that critical test coverage is maintained elsewhere.
// Licensed to the .NET Foundation under one or more agreements.

@@ -641,7 +641,7 @@ public static RuntimeTypeHandle GetNullableType(RuntimeTypeHandle nullableType)
/// </summary>
/// <param name="ip">Address inside the module</param>
/// <param name="moduleBase">Module base address</param>
public static unsafe string? TryGetFullPathToApplicationModule(IntPtr ip, out IntPtr moduleBase)
public static unsafe string TryGetFullPathToApplicationModule(IntPtr ip, out IntPtr moduleBase)
Copy link
Preview

Copilot AI Mar 6, 2025

Choose a reason for hiding this comment

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

The method signature has been changed to return a non-nullable string, but the implementation may return null when moduleBase is IntPtr.Zero. Consider returning string.Empty instead to adhere to the non-nullable contract.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@jkoritzinsky
Copy link
Member

Can we add the "BaseDirectory must be a rooted path" test as part of this?

@MichalStrehovsky
Copy link
Member Author

Can we add the "BaseDirectory must be a rooted path" test as part of this?

Why part of this? Revert PRs are typically just reverts. If we later revert the revert, it would revert the test too. I put it in #113232.

@jkoritzinsky
Copy link
Member

Adding it in a different PR is fine.

@MichalStrehovsky
Copy link
Member Author

/ba-g timeout in unrelated leg

@MichalStrehovsky MichalStrehovsky merged commit 9e3776e into main Mar 6, 2025
156 of 164 checks passed
@MichalStrehovsky MichalStrehovsky deleted the revert-112457-nativeaot-appcontext-base branch March 6, 2025 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants