docs: Add PACKAGING.md instructions#1342
Conversation
davidlattimore
left a comment
There was a problem hiding this comment.
Thanks for writing this up!
It could be good to also invite packagers to file an issue or otherwise reach out if they have any pain points that we might be able to help with.
|
|
||
| To test built binary, you can use `cargo test`, preferably with the same configuration as the build | ||
| to avoid rebuilding. | ||
| The testsuite is configured with `test-config.toml` file (default values are used if absent). |
There was a problem hiding this comment.
Are these intended as separate paragraphs? Markdown will merge them together unless there's a blank line between them.
There was a problem hiding this comment.
Intellij by default splits sentences into new lines, I can disable this if you want.
I'm aware of how Markdown works, but maybe I should do splitting here and there? I'm terrible at writing documents...
There was a problem hiding this comment.
Ah, that makes sense then, so they're not intended to be separate paragraphs. Splitting sentences onto new lines seems odd to me, but I don't really mind. I'm sure now that I know what's happening, I'll get use to it.
I'll just disable the sentence splitting, set splitting after 100th character and reflow it.
There was a problem hiding this comment.
Turns out I just enabled the wrong setting in the options 🤦🏻♂️
Reflowed the text.
|
|
||
| This project uses Cargo as its build system. | ||
| Wild's official artifacts are built using with `--profile dist` that enables compiler's internal | ||
| ThinLTO and strips the binaries. |
There was a problem hiding this comment.
It's funny. I'd thought for some time that our setting of lto = "thin" was redundant because I thought that was the default for optimised builds, however upon reading the docs again, I think I was mistaken. It sounds like there's one more LTO option than I had realised. Seems like the choices for LTO are none, thin-crate-local, thin-global, fat or linker-plugin-lto. I hadn't realised the distinction between thin-crate-local and thin-global (my names, not official names).
There was a problem hiding this comment.
Ah, that makes sense then, so they're not intended to be separate paragraphs. Splitting sentences onto new lines seems odd to me, but I don't really mind. I'm sure now that I know what's happening, I'll get use to it.
There was a problem hiding this comment.
Yeah, you can look at https://nnethercote.github.io/perf-book/build-configuration.html#link-time-optimization for the terminology.
In C/C++ world, linker is also involved in ThinLTO in some manner (LLD has some thinlto-* argsand it works only with Gold and LLD). Also, everyone has different idea what each LTO level means to add to the confusion: https://wiki.gentoo.org/wiki/LTO#Terminology
I'm not sure if my "compiler's internal ThinLTO" is correct. I mean, the linker is not involved in LTO at all, unlike in Clang's case.
davidlattimore
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me
|
|
||
| ## Binaries | ||
|
|
||
| This repository consists of Wild linker and linker-diff binaries. We recommend providing `wild` |
There was a problem hiding this comment.
Insert "the" before "Wild"?
There was a problem hiding this comment.
Coming from a language without the concept of "the", I doubt I'll ever get it right. Don't hesitate to point it out more.
| ## Testing | ||
|
|
||
| To test built binary, you can use `cargo test`, preferably with the same configuration as the build | ||
| to avoid rebuilding. The testsuite is configured with `test-config.toml` file (default values are |
There was a problem hiding this comment.
Since packaging contributors may not always be familiar with Wild, I think it would be more helpful if a note was added indicating that detailed information about each test option can be found at here.
| To test built binary, you can use `cargo test`, preferably with the same configuration as the build | ||
| to avoid rebuilding. The testsuite is configured with `test-config.toml` file (default values are | ||
| used if absent). To tweak the configuration, you can copy `test-config.toml.example` to | ||
| `test-config.toml` and edit it to your liking. Just be careful with `run_all_diffs` option, it's |
There was a problem hiding this comment.
(This may not directly relate to this section, but) clang-format tests are currently only able to be ignored by setting the WILD_TEST_IGNORE_FORMAT environment variable. This is mentioned in here, but might be easily overlooked at first glance. Since these tests could potentially fail depending on the clang version in the packaging environment, it might be worth considering whether clang formatting checks could be included in run_all_diffs (though this would likely need discussion), or at least explicitly mentioning the existence of this environment variable in the documentation.
There was a problem hiding this comment.
Good idea, we should make it more straightforward, I don't have preference for a particular solution.
There was a problem hiding this comment.
I believe we should determine how to handle clang-format diffs before the next release, since packaging typically occurs for already-released versions. Unless there are strong objections, I tend to include clang-format diffs in the run_all_diffs scope in a separate PR. The description in test-config.toml.sample would need to be updated in that case anyway, and I can take care of the whole thing.
There was a problem hiding this comment.
For packaging, we generally don't want to run any kind of lint tests (and clang-format would count as one). If setting the env var is required for that, the doc should mention it.
There was a problem hiding this comment.
I agree that controlling the clang-format test from test-config.toml makes sense. I'd perhaps make it a separate option, but I don't feel strongly.
Relatedly, I'm wondering if we should add a profile option to the test config which affects defaults. Possibly values might be "developer" or "unspecified", with "unspecified" being what you get if you don't have a config and so would apply to package builders. If we add new options that we think active developers should have on by default, we can set them in that profile's defaults. Not sure if we want to do that or not - just an idea.
There was a problem hiding this comment.
Added info about the environment variable. While configuring it via test-config.toml would be better, it's outside of this PR scope.
lapla-cogito
left a comment
There was a problem hiding this comment.
Thanks! Looks good to me.
TODO: replace this description before merging
Likely needs corrections and changes. Don't be afraid to post your ideas and thoughts.
I'm planning to leave it open for at least a week, but we can prolong that if needed.
cc @davidlattimore @marxin @lapla-cogito