Skip to content

Conversation

@tmds
Copy link
Member

@tmds tmds commented Jun 20, 2023

@tmds tmds requested a review from a team as a code owner June 20, 2023 11:51
@steveisok steveisok requested a review from directhex June 20, 2023 12:36
@premun
Copy link
Member

premun commented Jun 20, 2023

I am looking into why the repo didn't get initialized correctly in the build as this is expected to work

@premun
Copy link
Member

premun commented Jun 20, 2023

Fix is here - dotnet/arcade-services#2689

<Sha>4234ffac87e305da80597cb46dc0d4a87fc4f7c2</Sha>
<SourceBuild RepoName="xliff-tasks" ManagedOnly="true" />
</Dependency>
<Dependency Name="runtime.linux-x64.Microsoft.NETCore.Runtime.ObjWriter" Version="8.0.0-preview.6.23316.4" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be defined within the runtime's version.details.xml. Additionally the use of SourceBuildTarball should be updated to just SourceBuild. I am trying to remove the SourceBuildTarball attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied it from the old PR. I will update it.

I have no idea what this does. Can you give a short description how this is used, or point me to some doc?

Copy link
Member Author

@tmds tmds Jun 20, 2023

Choose a reason for hiding this comment

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

Ideally this would be defined within the runtime's version.details.xml.

You want me to make a PR against https://github.com/dotnet/runtime?
and add

    <Dependency Name="runtime.linux-x64.Microsoft.NETCore.Runtime.ObjWriter" Version="8.0.0-preview.6.23316.4" CoherentParentDependency="Microsoft.NETCore.App.Runtime.win-x64">
      <Uri>https://github.com/dotnet/llvm-project</Uri>
      <Sha>30e9e6bc2e9f04e0a75daf4b8088ee91f66069da</Sha>
    </Dependency>

to its eng/Version.Details.xml?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@MichaelSimons MichaelSimons Jun 20, 2023

Choose a reason for hiding this comment

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

Can you give a short description how this is used

It does two things.

  1. It indicates that the dependency should be included in the VMR. It specified the digest to include. The version details graph is walked and the first entry wins if there are multiple for a repo.

  2. It tells the source build infra to pull in the intermediate package for the repo when building the repo level source build. The intermediate contents pre-populate the local nuget cache for prebuilt detection.

It looks like these are already defined:

One will need the SourceBuild element added to it.

Copy link
Member Author

@tmds tmds Jun 20, 2023

Choose a reason for hiding this comment

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

One will need the element added to it.

I don't understand what you mean.

got it, the SourceBuild element.

Copy link
Member

Choose a reason for hiding this comment

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

Apologies, I used angle brackets in my original response and it wasn't rendered. I updated my comment.

@premun
Copy link
Member

premun commented Jun 20, 2023

@tmds @MichaelSimons I don't have a fix for the problem and I don't know what's going wrong there but.. I won't have time until Friday to fix this.

Can we, as a workaround (and reading the PR here too), check this in without the <SourceBuild> tag? This would only add the source-mappings.json record and not initialize the repository. I would then do that manually through a PR into dotnet/dotnet to unblock this.

We can then wait until the source build tag propagates from runtime where we want to put it anyway.

@MichaelSimons
Copy link
Member

I have no objection to @premun's proposed workaround.

@tmds
Copy link
Member Author

tmds commented Jun 21, 2023

check this in without the <SourceBuild> tag?

Was this handled by 246798d, or is there something else I should do?

@premun
Copy link
Member

premun commented Jun 21, 2023

Ah, it's now failing because the repo project cannot build the folder that is not there.

I think we'd need to do this in 3 stages:

  • Add the source-mappings.json record only
  • Initialize the LLVM repo in the VMR through a PR to dotnet/dotnet
  • Add the repo project and everything else

Question - we first need to do dotnet/llvm-project#439 anyway, right?

@tmds
Copy link
Member Author

tmds commented Jun 21, 2023

Add the source-mappings.json record only

Created #16768.

Initialize the LLVM repo in the VMR through a PR to dotnet/dotnet

I assume we don't need to wait for dotnet/llvm-project#439 to do this? The sources can be added and won't get compiled until we add this PR.

@premun can you add the LLVM repo in the VMR?
Is the source-mappings.json update needed for that?

@premun
Copy link
Member

premun commented Jun 21, 2023

So.. I investigated this a bit and git is unable to apply a patch representing the LLVM repo. It only returns a generic error:

error: git apply: failed to read: No such file or directory

I could initialize the repo more manually but looking at it, htere's a high number of quite large binaries so I don't think we should just do it without a proper preparation and cloaking rule setup.

@MichaelSimons
Copy link
Member

I could initialize the repo more manually but looking at it, htere's a high number of quite large binaries so I don't think we should just do it without a proper preparation and cloaking rule setup.

Agreed - the repo is 3.0 G on disk.

@jkotas
Copy link
Member

jkotas commented Jun 22, 2023

All binaries that I can see are under tests. It should be fine to cloak all test directories.

@jkotas
Copy link
Member

jkotas commented Jun 22, 2023

Also, it should be ok to cloak the following top-level directories:

libclc
lld
lldb
mlir
openmp
polly
third-party

@directhex
Copy link

Also, it should be ok to cloak the following top-level directories:

libclc
lld
lldb
mlir
openmp
polly
third-party

Do we want the non-cloaked folders to cover only CoreCLR's needs (i.e. objwriter's deps) or also Mono? We need to keep lld for Mono. I think bolt, flang and pstl can go though?

@jkotas
Copy link
Member

jkotas commented Jun 22, 2023

We need to keep lld for Mono.

I did not know that. We should keep lld for Mono then.

@omajid
Copy link
Member

omajid commented Jun 22, 2023

We should an issue to track/fix all the UX issues related to adding a new repo to the VMR. @tmds, is that something you can do?

@tmds
Copy link
Member Author

tmds commented Jun 22, 2023

We should an issue to track/fix all the UX issues related to adding a new repo to the VMR. @tmds, is that something you can do?

We're looking into fixing the issues as we encounter them. If there are things we're not fixing now, I think @premun and @MichaelSimons will create issues to track them.

@premun
Copy link
Member

premun commented Jun 23, 2023

I will start looking today into the failing git apply as well as the right way to cloak the llvm-project repository

@tmds
Copy link
Member Author

tmds commented Jun 23, 2023

@premun thanks. I will let you and @MichaelSimons add the llvm-project in the vmr, and once it's in there I'll continue working towards dotnet/source-build#1215.

@premun
Copy link
Member

premun commented Jun 23, 2023

@directhex what is the right way to test whether the trimmed llvm repo works for Mono?

@premun
Copy link
Member

premun commented Jun 23, 2023

Looking at the build, to make this work, we'd have to temporarily add the <SourceBuild> tag back so that the LLVM repo is part of the dependency tree here. That is if we want to use this PR for prototyping this.

@tmds
Copy link
Member Author

tmds commented Jun 23, 2023

@premun since I have this PR open, I assume you will have permissions to push to my branch. Feel free to do so.

@premun
Copy link
Member

premun commented Jun 23, 2023

Seems like pstl is needed:

##[error]/vmr/artifacts/source-built-sdks/Microsoft.DotNet.Arcade.Sdk/tools/SourceBuild/SourceBuildArcadeBuild.targets(121,5): error MSB3026: Could not copy "/vmr/src/llvm-project/libcxx/test/std/pstl" to "/vmr/src/llvm-project/artifacts/source-build/self/src/libcxx/test/std/pstl". Beginning retry 6 in 1000ms. Could not find file '/vmr/src/llvm-project/libcxx/test/std/pstl'.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=318493&view=logs&j=941f2e24-48a0-594a-6b1a-0e16aaeb8606&t=25681878-3d3d-50c4-a78d-b74bb03f4ec7

@premun
Copy link
Member

premun commented Jun 23, 2023

I've done few more investigations and, interestingly, if the /lldb directory is included, then git diff + git apply fails.
If I exclude /lldb, it works. If I sync only /lldb, it also works. It only breaks when I sync the whole repo and git apply fails.

I suspect this is a bug in git itself. I posted a bugreport to the git mailing list but had other fires today so didn't spend more on this.

But right now, it's failing on some build error that I will let @directhex or possibly someone else hash out.

EDIT: These are the largest files in lldb, a possibility:

8.9M    ./unittests/Process/minidump/Inputs/fizzbuzz_wow64.dmp
8.9M    ./test/API/functionalities/postmortem/wow64_minidump/fizzbuzz_wow64.dmp
476K    ./source/Plugins/Instruction/ARM/EmulateInstructionARM.cpp
368K    ./source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
324K    ./test/API/functionalities/postmortem/elf-core/linux-ppc64le.core
292K    ./source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
256K    ./source/Core/IOHandlerCursesGUI.cpp
248K    ./test/API/functionalities/postmortem/netbsd-core/2lwp_t2_SIGSEGV.aarch64.core
244K    ./test/API/functionalities/postmortem/netbsd-core/2lwp_process_SIGSEGV.aarch64.core
228K    ./tools/debugserver/source/RNBRemote.cpp
228K    ./source/Target/Process.cpp
224K    ./test/API/functionalities/postmortem/netbsd-core/1lwp_SIGSEGV.aarch64.core
216K    ./source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
188K    ./source/Commands/CommandObjectTarget.cpp
180K    ./source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
168K    ./source/Target/Target.cpp
156K    ./tools/debugserver/source/MacOSX/MachProcess.mm
152K    ./source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
152K    ./source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
148K    ./test/API/functionalities/postmortem/elf-core/linux-aarch64-pac.core
144K    ./test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-full.core
144K    ./test/API/functionalities/postmortem/elf-core/linux-aarch64-sve-fpsimd.core
144K    ./source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
140K    ./source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
128K    ./source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
128K    ./source/Interpreter/CommandInterpreter.cpp
124K    ./tools/lldb-vscode/lldb-vscode.cpp
120K    ./test/API/functionalities/postmortem/netbsd-core/2lwp_t2_SIGSEGV.amd64.core
120K    ./test/API/functionalities/postmortem/netbsd-core/2lwp_process_SIGSEGV.amd64.core
120K    ./source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
120K    ./include/lldb/Target/Process.h
116K    ./tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp
112K    ./source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
112K    ./source/Core/ValueObject.cpp
108K    ./unittests/UnwindAssembly/x86/Testx86AssemblyInspectionEngine.cpp
108K    ./unittests/SymbolFile/PDB/Inputs/test-pdb.pdb
108K    ./source/Expression/DWARFExpression.cpp
104K    ./source/Commands/CommandObjectType.cpp
104K    ./packages/Python/lldbsuite/test/lldbtest.py
100K    ./unittests/SymbolFile/PDB/Inputs/test-pdb-types.pdb
100K    ./test/Shell/Minidump/Windows/Sigsegv/Inputs/sigsegv.pdb
100K    ./test/Shell/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp
100K    ./test/API/functionalities/postmortem/netbsd-core/1lwp_SIGSEGV.amd64.core

@tmds
Copy link
Member Author

tmds commented Jun 23, 2023

But right now, it's failing on some build error that I will let @directhex or possibly someone else hash out.

It looks like something the changes made in dotnet/llvm-project#439 to llvvm.proj would fix.

@premun
Copy link
Member

premun commented Jun 26, 2023

Ok, so got in touch with git maintainers and the problem with git not processing the patch is that in October, they introduced a 1GB cap on patch size due to some problems with types they use during parsing.

  • The changed happened here: git/git@f1c0e3946e
  • I was suggested to use git-repo-filter which is a python tool that is supposed to work faster but we could only use that during initialization, not VMR synchronization.
  • git maintainers will fix the error to contain the real reason but there is no override as it would mean fixing quite a few things on their side first.

I am not in favour of bringing python into the mix so I think we should just detect >1GB patches and in that case, we can just piece the patches by top-level folder (recursively) and solve this.
That being said, the cloaked llvm repo now fits just under 1GB (it's a bit over 900MB now) so it's not strictly blocking and we're not losing any files or so. I will log an issue for this improvement and hopefully we don't initialize any huge repo soon.

@tmds
Copy link
Member Author

tmds commented Jul 4, 2023

@premun what is still needed to finish this PR?

@premun
Copy link
Member

premun commented Jul 4, 2023

@premun what is still needed to finish this PR?

  1. I don't have full context on how llvm-project is used in the build. My understanding is that runtime uses it?
  2. The repo that actually depends on llvm-project should have the appropriate record in its Version.Details.xml declared as a source build dependency via the SourceBuild tag. I don't know if that should be runtime then?
  3. The dependency should then point at the SHA that we care about - is that the new versioned-main commit I saw?
  4. We should remove the tag that we added here artificially and flow here 'runtime' which has the SourceBuild tag inside.
  5. We should make sure in this PR that the cloaking rules for LLVM do not omit something that breaks the build by building whatever configuration that uses LLVM in a PR build here.
  6. Possibly, we could even run a full VMR build with all SB legs (might need to do this internally) to make sure we don't break other builds.

@tmds
Copy link
Member Author

tmds commented Jul 4, 2023

Can we start by getting the llvm-project into the vmr so it gets built but without wiring it up further?

For that, I believe we need to finish this PR, and dotnet/llvm-project#439?
Is that right? Or do we need something more?

@premun
Copy link
Member

premun commented Jul 4, 2023

Yes, that PR has to go through so that we can verify that it source-builds at the repo level. There's no point adding the repo in the VMR yet though. Or is there a reason for that?

Personally, I would do that as late as possible. We were already about to do that once and didn't end up doing that. I think adding the repo will increase the size of the repo quite a lot so until we can plug it in and start using it, I'd rather wait.

@tmds
Copy link
Member Author

tmds commented Jul 4, 2023

Yes, that PR has to go through so that we can verify that it source-builds at the repo level. There's no point adding the repo in the VMR yet though. Or is there a reason for that?

I'm still stuck at get the llvm-project in the vmr (dotnet/source-build#1215 (comment)).

@tmds
Copy link
Member Author

tmds commented Feb 2, 2024

We no longer need to integrate llvm-project to have NativeAOT in .NET 9.

@tmds tmds closed this Feb 2, 2024
@mthalman
Copy link
Member

mthalman commented Feb 2, 2024

We no longer need to integrate llvm-project to have NativeAOT in .NET 9.

Why is that?

@steveisok
Copy link
Member

We no longer need to integrate llvm-project to have NativeAOT in .NET 9.

Why is that?

They switched to a c# version of the objwriter.

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.

8 participants