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

Disable NAOT native exports for non-publish builds #1547

Draft
wants to merge 1 commit into
base: staging/AOT
Choose a base branch
from

Conversation

Sergio0694
Copy link
Member

Fixes #1546

This PR disables the NAOT native exports and restores the bundled native host for non-publish builds.

@Sergio0694 Sergio0694 added bug Something isn't working authoring Related to authoring feature work AOT labels Mar 21, 2024
@Sergio0694 Sergio0694 changed the base branch from master to staging/AOT March 21, 2024 04:33
@MichalStrehovsky
Copy link
Member

This looks good to me in the sense that I don't think there's any other way to do what you need. _IsPublishing is obviously an internal implementation detail of the SDK, but the SDK doesn't provide any other way to do this so...

Cc @baronfel @nagilson

@nagilson
Copy link
Member

Yes, keep in mind this will not work for ms build t publish, only when running dotnet publish on CLI (and eventually VS publish, but not at the moment.)

@Sergio0694
Copy link
Member Author

Mmh when it "doesn't work", does that mean that _IsPublishing would just not be set to anything? Because I'm wondering if that's the case, perhaps would it be better for us to check for '$(_IsPublishing)' != 'false'" rather than '$(_IsPublishing)' == 'true'"? The rationale being that with the former, dotnet publish would work the same, but msbuild -t:publish would then also work (as the property would just not be set to anything). Basically it'd be the same as today, except we'd get the small improvement that dotnet build would correctly no longer generate those exports (as _IsPublish would presumably be false then?). Thinking that might be better than just outright breaking consumers using msbuild to publish, since that's relatively common. Does that make sense? 🤔

@MichalStrehovsky
Copy link
Member

I don't think you'll ever see _IsPublishing == false.

dotnet publish sets the property to true. Publish in VS currently doesn't. They're working on making it set it. dotnet build /t:Publish would not have it either and will still do a publish (with some extra hurdles, but it will do a publish too).

@nagilson
Copy link
Member

I don't think you'll ever see _IsPublishing == false.

dotnet publish sets the property to true. Publish in VS currently doesn't. They're working on making it set it. dotnet build /t:Publish would not have it either and will still do a publish (with some extra hurdles, but it will do a publish too).

It's a good thought, but @MichalStrehovsky is right, currently thats pretty much never false.

@Sergio0694
Copy link
Member Author

I see. Thank you both for the additional info! I'll revert this to draft. Seems like we just can't do it for now 😅

@Sergio0694 Sergio0694 marked this pull request as draft April 29, 2024 22:55
@nagilson
Copy link
Member

Thank you for checking in with us! I think its a good idea. If youre ok with it not being the same in msbuild t publish, then I dont see why we wouldnt merge this once vs has the behavior, which id expect sooner rather than later.

@agocke is tracking that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOT authoring Related to authoring feature work bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CsWinRTAotExportsEnabled incorrectly assumes PublishAot == true implies a publish is actually occurring
3 participants