-
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
Get core-setup building in the consolidated repo. #2
Conversation
I've discovered a few TODOs for things that wouldn't show up on a local build. I'll keep a running list here as I discover them.
|
eng/Build.props
Outdated
@@ -90,7 +90,7 @@ | |||
|
|||
<Target Name="GetRepoTasksSrc"> | |||
<PropertyGroup> | |||
<RepoTasksDir>$(RepoRoot)tools-local\tasks\</RepoTasksDir> | |||
<RepoTasksDir>$(RepoRoot)src\setup\tools-local\tasks\</RepoTasksDir> |
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.
We should paths for the three different repos in a Directory.Build.props file in the repo root:
<Project>
<PropertyGroup>
<SourceDir>$(RepoRoot)src\</SourceDir>
<SetupRootDir>$(SourceDir)setup\</SetupRootDir>
<RuntimeRootDir>$(SourceDir)runtime\</RuntimeRootDir>
<LibrariesRootDir>$(SourceDir)libraries\</LibrariesRootDir>
</PropertyGroup>
</Project>
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.
Of course that file would need to be imported manually as Arcade doesn't import the Directory.Build.props from the repo in the override files like Tools.props, Build.props.
src/libraries/build.cmd
Outdated
|
||
:end | ||
exit /b %ERRORLEVEL% | ||
%~dp0\..\..\build.cmd /p:Subset=libraries %* |
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.
if those two scripts are the only ones with the hardcoded relative parent paths then it should be fine.
@@ -5,7 +5,7 @@ | |||
Condition="'$(SkipSigning)' != 'true' and '$(SignType)' != 'public'"> | |||
<Message Importance="High" Text="Signing final packages" /> | |||
<MSBuild | |||
Projects="$(RepoRoot)signing\SignFinalPackages.proj" | |||
Projects="$(SetupRoot)signing\SignFinalPackages.proj" |
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 would be nice if the signing directory in core-setup would also be under core-setup/eng/
DestinationFiles="$(PublishRootDir)LICENSE.txt" | ||
Condition="'$(TargetsUnix)' == 'true'"/> | ||
|
||
<Copy SourceFiles="$(ProjectDir)resources/LICENSE-MSFT.txt" | ||
<Copy SourceFiles="$(SetupRoot)resources/LICENSE-MSFT.txt" |
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.
should we move this file to a more common place in the repo root / repo subdir?
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.
maybe that's better to discuss at a later point.
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.
A couple of doc nits, looking very reasonable to me overall.
Signing hooks need to be re-enabled in a way that doesn't break the other repos.
Let me know how this breaks (once it's something that needs to be worked on), I'm responsible for how complicated it ended up in core-setup. 😄
Determine what should happen with the Tools.props file in setup/eng.
I'm honestly not sure what that file is for. May be something to clarify with Arcade folks if nobody already has context. (E.g. what they're for in CoreFX's more extensive copy of the file. It looks like CoreFX's is a superset?)
Yes, taking the super-set from corefx with the merge from coreclr should be fine. (Tools.props) |
@dagood In the current state of the repo, the |
Backport to core-setup PR: dotnet/core-setup#8593 Once that's merged, I'll put out a PR to dotnet/consolidation with a patch file and template updates for the root folder so the next scouting exercise we do will have all of my work included. |
Small changes from Jarret
Search for CoreLib in TPA
This reverts commit a09b4d3, partially, all but Huffman.cs
These were the changes I needed to make to get core-setup building in the consolidated repo, as well as some suggestions from @ViktorHofer.
eng/Build.props
. MoveSubset.props
to the rooteng
directory.$(RepoRoot)
to a new MSBuild property$(SetupRoot)
which points to the root of the setup directory.I felt it was easiest to allow the setup build to output to the root
artifacts
directory as per Arcade convention, so there were a few changes I needed to make to enable the tests to work from there:I'll start working on porting back relevant pre-work back to core-setup, and ready a patch to add to dotnet/consolidation for changes that are only relevant after migration.