-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Creating a runtime pack artifacts directory in the libraries build #34662
Creating a runtime pack artifacts directory in the libraries build #34662
Conversation
Tagging @ViktorHofer as an area owner |
We should follow this model and set something like:
|
42a7587
to
b17e35d
Compare
b17e35d
to
c3ba797
Compare
@@ -2,6 +2,8 @@ | |||
|
|||
<PropertyGroup> | |||
<TraversalGlobalProperties>BuildAllProjects=true</TraversalGlobalProperties> | |||
<NETCoreAppFrameworkIdentifier>.NETCoreApp</NETCoreAppFrameworkIdentifier> | |||
<SharedFrameworkName>Microsoft.NETCore.App</SharedFrameworkName> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Share values with installer project tree? (Move to common ancestor?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would be a good place to place these properties in <runtimeroot>/src/Directory.Build.props
?
Is it a significant impact to performance/organization to move it up? I'm not familiar enough to know now if those properties have a chance of changing or differing between the directories in <runtimeroot>/src
. If they were to differ later would it be better to have each specify their own properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not be worth it. My knee-jerk reaction is that duplicating code is what needs justification rather than reusing it, so just a quick suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's open an issue for this. Consolidate all places in props and targets where we use Microsoft.NETCore.App to a single shared property.
86833e0
to
3cafc0f
Compare
…d remove exclusion in runtime.depproj
3ea474a
to
0c650a3
Compare
@@ -73,6 +70,20 @@ | |||
<Exec Command="chmod +x $(TestHostRootPath)%(DotnetExe.Filename)%(DotnetExe.Extension)" Condition="'$(TargetOS)' != 'Windows_NT' and '$(OS)' != 'Windows_NT'"/> | |||
</Target> | |||
|
|||
<Target Name="SetupRuntimePackNative" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this target is specific to Mono
should we include Mono
in the target name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are going to be some differences with desktop mono, so I'd like to address them in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an issue tracking this work we can reference here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5920234
to
fe67b95
Compare
src/libraries/pretest.proj
Outdated
|
||
<PropertyGroup> | ||
<TraversalGlobalProperties>BuildAllProjects=true</TraversalGlobalProperties> | ||
<NETCoreAppFrameworkIdentifier>$(NETCoreAppFrameworkIdentifier)</NETCoreAppFrameworkIdentifier> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is that property NETCoreAppFrameworkIdentifier
defined in libraries? I don't see it anywhere. I was suggesting to use TargetFrameworkIdentifier
which should contain the right value here but I might be wrong (if this project doesn't have a target framework set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move the already defined property in installer into Configurations.props: https://github.com/dotnet/runtime/blob/master/eng/Configurations.props#L25 as it then will be available here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I also didn't find it in libraries, but I saw that it was used in
<FrameworkListRootAttributes Include="TargetFrameworkIdentifier" Value="$(NETCoreAppFrameworkIdentifier)" />
and I accidentally picked the wrong one. changing to TargetFrameworkIdentifier
9dcf2f4
to
ba6d021
Compare
src/libraries/pretest.proj
Outdated
@@ -1,7 +1,11 @@ | |||
<Project Sdk="Microsoft.Build.Traversal"> | |||
<Import Project="Sdk.props" Sdk="Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk" /> | |||
<Import Project="Sdk.targets" Sdk="Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The targets are usually imported at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I left an additional comment. The Sdk.targets in pretest.proj should be imported at the end.
Also, let's open an issue to consolidate Microsoft.NETCore.App usage in our props and targets to share the same property.
I think it is worth opening an issue to cleanup the TargetOS == iOS or ...
conditions to use TargetsMobile == true
.
src/libraries/pretest.proj
Outdated
@@ -94,4 +91,7 @@ | |||
TargetFilePrefixes="ref/;runtimes/" | |||
RootAttributes="@(FrameworkListRootAttributes)" /> | |||
</Target> | |||
|
|||
<Import Project="Sdk.props" Sdk="Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props go at the top of the file, the targets at the end 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh oops, thanks!
405b950
to
0d46895
Compare
The 2 remaining lanes seem stuck. All the lanes that this change impacts have passed, so merging. |
This should not be a reason for merging. This should be a reason to ping the infra team to look into the issue. In the last few days we've had a series of PRs merged without full validation. We're also running at 0% passing in CI. These are completely related items. |
@jaredpar there was already a Teams thread going on about this and https://github.com/dotnet/core-eng/issues/9630 is tracking it. |
This PR looks to create a runtime package in the
artifacts/bin
directory through the libraries build process for the purpose testing the mobile apps (iOS and Android).The desired directory hierarchy is as follows