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

Enable StripSymbols by default #85139

Merged
merged 4 commits into from Apr 23, 2023
Merged

Conversation

MichalStrehovsky
Copy link
Member

See discussion in dotnet/sdk#31739 (comment).

Once this merges, we should:

  • Update the dotnet new console -aot template
  • Update the dotnet new api -aot template
  • Update docs and declare different defaults in 8.0.

Cc @dotnet/ilc-contrib @am11 @DamianEdwards @eerhardt

See discussion in dotnet/sdk#31739 (comment).

Once this merges, we should:
- [ ] Update the `dotnet new console -aot` template
- [ ] Update the `dotnet new api -aot` template
- [ ] Update docs and declare different defaults.
@ghost
Copy link

ghost commented Apr 21, 2023

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

Issue Details

See discussion in dotnet/sdk#31739 (comment).

Once this merges, we should:

  • Update the dotnet new console -aot template
  • Update the dotnet new api -aot template
  • Update docs and declare different defaults in 8.0.

Cc @dotnet/ilc-contrib @am11 @DamianEdwards @eerhardt

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky MichalStrehovsky added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 21, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 21, 2023
@ghost
Copy link

ghost commented Apr 21, 2023

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking 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.

Thank you!

@MichalStrehovsky
Copy link
Member Author

Cc @kotlarmilos @akoeplinger looks like there's an issue with the GenerateUnmanagedEntrypoints test when stripsymbols is specified. A wild guess is that this strips the symbols we actually need. Don't have a mac around to investigate. I'm going to enable StripSymbols on the test on mac so that we don't block this since GenerateUnmanagedEntrypoints is not an end user scenario.

      BEGIN EXECUTION
      /tmp/helix/working/A662095E/p/nativeaottest.sh /private/tmp/helix/working/A662095E/w/B46609F8/e/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints/ GenerateUnmanagedEntryPoints.dll ''
      Expected: 100
      Actual: 2
      END EXECUTION - FAILED

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit 8c2c15b into dotnet:main Apr 23, 2023
188 of 196 checks passed
@MichalStrehovsky MichalStrehovsky deleted the stripsym branch April 23, 2023 07:24
@MichalStrehovsky MichalStrehovsky removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 24, 2023
@ivanpovazan
Copy link
Member

Cc @kotlarmilos @akoeplinger looks like there's an issue with the GenerateUnmanagedEntrypoints test when stripsymbols is specified. A wild guess is that this strips the symbols we actually need. Don't have a mac around to investigate. I'm going to enable StripSymbols on the test on mac so that we don't block this since GenerateUnmanagedEntrypoints is not an end user scenario.

      BEGIN EXECUTION
      /tmp/helix/working/A662095E/p/nativeaottest.sh /private/tmp/helix/working/A662095E/w/B46609F8/e/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints/ GenerateUnmanagedEntryPoints.dll ''
      Expected: 100
      Actual: 2
      END EXECUTION - FAILED

I can look into this, as I added the test case.

@kotlarmilos
Copy link
Member

Cc @kotlarmilos @akoeplinger looks like there's an issue with the GenerateUnmanagedEntrypoints test when stripsymbols is specified. A wild guess is that this strips the symbols we actually need. Don't have a mac around to investigate. I'm going to enable StripSymbols on the test on mac so that we don't block this since GenerateUnmanagedEntrypoints is not an end user scenario.

      BEGIN EXECUTION
      /tmp/helix/working/A662095E/p/nativeaottest.sh /private/tmp/helix/working/A662095E/w/B46609F8/e/nativeaot/GenerateUnmanagedEntryPoints/GenerateUnmanagedEntryPoints/ GenerateUnmanagedEntryPoints.dll ''
      Expected: 100
      Actual: 2
      END EXECUTION - FAILED

I can look into this, as I added the test case.

@ivanpovazan Thanks! Let me know if the failure is caused by particular stripping options introduced in #81361.

@dotnet dotnet locked as resolved and limited conversation to collaborators May 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants