feat: Wire up LLD tests via wild-linker/lld-tests submodule - #2399
feat: Wire up LLD tests via wild-linker/lld-tests submodule#2399deepakshirkem wants to merge 1 commit into
Conversation
marxin
left a comment
marxin
left a comment
There was a problem hiding this comment.
Frankly speaking, I don't like us taking the copy of the LLVM's lld/test/ELF sub-folder and tracking them as part of the main git repository. If we include all files in the aforementioned folder, we're speaking about ~2400 files.
I would rather see us using an existing submodule https://github.com/wild-linker/test-files, where we can also put the synchronization script.
@marxin That makes sense to me. @davidlattimore what you think? |
Hmm, that is quite a few. The other option would be for us to create a new repository under the wild-linker org and put it in there, then make that be a git submodule. That'd make it more like how we do for the mold tests, but instead of using the upstream repo as a submodule, we'd be using our own cut-down copy of it due to size reasons. But we'd need a name - perhaps "lld-tests"? |
Works for me! I'm fine with us doing one more git submodule (under wild-linker org) and using it. The repository might be easily just a copy of the LLVM subfolder ( Note the folder contains ~4K files. |
|
A few name options, no strong preference either way: lld-tests Once the repo's up, I'll rework this PR to pull from it as a submodule, same as external_test_suites/mold. |
|
I've created https://github.com/wild-linker/lld-tests. |
4b234ee to
2967ce4
Compare
deepakshirkem
changed the title
|
@marxin Can you take another look? I've made this PR submodule-specific now if there's anything else you think I should remove. |
marxin
left a comment
marxin
left a comment
There was a problem hiding this comment.
I think we should include running of a selected subset of tests as part of this PR - either by explicitly listing them, or similarly to Mold, by skipping the unsupported ones.
| return Ok(()); | ||
| } | ||
|
|
||
| // Test activation is not yet implemented. The submodule wiring |
There was a problem hiding this comment.
Well, it does not make much sense to merge the PR that is actually not running any of the tests.
| Ok(()) | ||
| } | ||
|
|
||
| fn load_skip_tests_config() -> &'static Option<Vec<String>> { |
There was a problem hiding this comment.
Similarly here, it's a code that is not actually run.
2967ce4 to
be4d64a
Compare
Replaces directly-committed test files with a proper git submodule pointing at wild-linker/lld-tests, which vendors the full lld/test/ directory with its own sync script. Test activation is intentionally left as a no-op for now — the full LLD suite includes many untriaged tests, and deciding what should run vs. be skip-listed is left to a follow-up PR.
be4d64a to
07ff4c9
Compare
|
@marxin I added the tests, but I wanted to confirm with you as well — is my understanding of these tests correct? |
davidlattimore
left a comment
davidlattimore
left a comment
There was a problem hiding this comment.
Do you think you could split adding the git submodule out to a separate PR and get that in first? Especially if we might iterate on this a bit. I often like to pull down PRs to try them out, but having a submodule addition makes it tricky because if I initialise the submodule, then once I move off the commit that adds it, things get messy. Basically it'd be good to get the submodule in quickly, then I can more easily review the remainder of the change.
I'm not sure that the "feat" prefix is ideal for this change. It would mean that this would appear in the "features" section of the release notes - but from the perspective of a user of the linker, this isn't a feature. "test" is probably more appropriate.
| ot = "ot" | ||
| FRE = "FRE" | ||
| aranges = "aranges" | ||
| preemptable = "preemptable" |
There was a problem hiding this comment.
This is a typo that I've been known to make myself, so it'd be good if we could avoid adding it here. I just did some experimentation, and it looks like we can skip typo checking specifically for the excluded test lists with something like this:
[type.external-test-list]
extend-glob = ["*_skip_tests.toml"]
extend-ignore-re = ['(?m)^\s*tests\s*=\s*\[[^\]]*\]\s*$']3 participants
Replaces the directly-committed test files from earlier commits on this PR with a proper git submodule pointing at wild-linker/lld-tests, which vendors the full
lld/test/directory (ELF, COFF, MachO, MinGW, Unit, wasm) and has its own sync script.Test activation is intentionally left as a no-op for now
collect_testsreturns early without registering any tests. The full LLD suite includes many untriaged tests, and deciding what should run vs. be skip-listed is left to a follow-up PR. This PR is scoped to just the submodule wiring.Part of #2380.