-
Notifications
You must be signed in to change notification settings - Fork 179
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
tc-build: Rewrite #226
tc-build: Rewrite #226
Conversation
I gave my regular old toolchain build a go and that's working again. I haven't tried the PGO one yet, I'll give it a go tomorrow (if I remember!) |
I'm not overly sold on the deprecation & reuse of arguments, but I don't really view the backwards compat of this script as being particularly important. -b -> -r, -B -> -b might do something weird for some people I suppose. I just can't bring myself to care? |
I would like to see that in an own Git branch. Maybe with LLVM |
Appreciate it!
Yeah, I am not the biggest fan of it either but I think the amount of options that the script has accrued over time is a little on the larger side so if I am already rewriting the scripts, it seems reasonable to do a one-time deprecation period. Perhaps we could add a one time notice to the script that will notify the user of the new options like:
I am not sure I understand this comment. Are you saying that even after this is merged, it should live a different branch than main? |
Alright, I believe this is ready for full testing, as As I mentioned above, if there are concerns with backwards compatibility, we could warn the user once then write a git ignored file to disk to avoid warning in the future. At the same time, if the user is pulling a new update, I would expect them to glance at the commit log to see what new changes they are getting. I do not feel strongly about it, I'll do whatever people feel is right. |
These scripts have gotten a little too unweildy, to the point where it is hard to follow the overall flow when making script wide changes. I have learned a lot about Python since I wrote this script so let's see if we can make something a little cleaner :) Signed-off-by: Nathan Chancellor <nathan@kernel.org>
The previous version of tc-build was hard to follow because data was passed from arguments down through a maze of functions, which made it hard to follow where decisions were being made about that data and make changes through the pipeline. This rewrite aims to avoid that complexity by having classes that encapsulate the logic of managing sources on disk and building these bits of software into clear and concise implementations. Now, the scripts focuses on handling the user's input, instantiating classes with the data provided or default data when necessary, then invoking class methods that work on this data internally, which makes it way simpler to see where things are getting modified or changed. Additionally, by using classes, we get the benefit of sharing more code than before, which makes everything much more maintainable in the long run. Right up front, this is documented a lot less through comments and docstrings than the previous iteration, as I wanted to try and make the logic self explanatory through descriptive class, method, and varviable names, but there could be places where there is still confusion. Holler if anything needs to be clarified via a code comment. Additionally, rather than being a 1:1 rewrite in terms of functionality, I made some internal changes that may have user visible effects: * Many options have dropped their default value in argparse, as they are options for a reason. This allows the script to easily determine what the user's intent is and make better internal decisions like managing sources. A notable option that no longer has a default value is '--install-folder'. Now, if '--install-folder' is not specified, the toolchain is just left alone in the build folder. I think this makes sense, as it allows us to eliminate '--install-stage1-only'. The user can now just specify '--build-stage1-only' and '--install-folder', resulting in a deterministic outcome. * Install folder is no longer specified by default, meaning that the toolchain just remains in the build folder once the build is completed. I think this makes more sense versus having a default installation folder but ignoring it for stage one but only if the user did not request installation. Now, if the user did not request explicit installation, they will not get it. * The '--projects' and '--targets' option now take a list of projects and targets, rather than the semicolon separated string that would be passed along directly to cmake. This allows for easy validation of targets and feels more natural with how option handling worked for the rest of the options. * Certain options have been eliminated that were deemed useless to port over: * '--incremental': I am not sure this has ever worked, as running cmake in a directory seems to be hit or miss at times. Getting rid of this option allows the script to elminate cleaning up previous versions of internal files. I do not expect eliminating this option to burden many people. If build time is a problem, the user is likely doing a single stage build, in which case they can take advantage of ccache. * '--install-stage1-only': As mentioned above, the presence of '--install-folder' is now what determines if the final stage is installed. * Certain options have been renamed due to dropping certain options or changing the meaning of the flag: * '-b' / '--branch' is now '-r' / '--ref'. '-b' is now short for '--build-folder' and '-B' is eliminated. * '--clang-vendor' is now '--vendor-string', as LLD supports a vendor string as well. * With '-i' / '--incremental' eliminated, '-I' / '--install-folder' now takes over that short option and '-I' is eliminated. * The name of the build folder of a particular stage is now a little bit more descriptive around that particular stage: "stage1" (normal) -> "bootstrap" "stage1" ('--build-stage1-only') -> "final" "stage2" (normal) -> "final" "stage2" ('--pgo') -> "instrumented" "stage3" ('--pgo') -> "final" * Fewer PGO kernel builds. Previously, if the user specified both 'kernel-defconfig' and 'kernel-allmodconfig', certain architectures would see duplicate builds because their 'allmodconfig'/'allyesconfig' is broken, so 'kernel/build.sh' would just build the defconfigs again, which unnecessarily extends the build time. To fix this, we have now a matrix of configuration targets ("defconfig", "allmodconfig", "allyesconfig"), which each have a list of builders. The LLVMKernelBuilder class handles generating this list of builders based on the desired configuration targets for benchmarking and the LLVM targets configured for the toolchain. A consequence of this matrix generation is that the situation of the user specifying both a "full" and "slim" configuration target becomes a little ambiguous; previously, they would have just both been built but now that we have this granularity available to us, it seems wise to take the conservative approach of honoring the "slim" variant and printing a warning. * binutils are no longer built when doing kernel builds if they are not found in PATH; instead, the kernel build is skipped with a warning. This is partially due to the installation folder change mentioned above since binutils would need to be installed to the kernel's build folder if no installation folder was specified, which would happen every time the user did PGO. This is wasteful in my opinion; the user should just install their distribution's bintuils or run build-binutils.py and pass that folder along via PATH: $ PATH=...:$PATH ./build-llvm.py ... It is not the end of the world if that kernel's backend does not get profiled. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
So you pushed several time your pull-226-patchset. I would like to see: tcbuild.github#Rewrite-v1 ...as branches beside main. Easier for me to compare with main v1 v2 etc.
BTW, |
GitHub's UI has
The previous versions were either buggy or incomplete, as I mention above. Consider this most recent push as Any future changes that are needed for this rewrite will be done as individual commits on top. |
LLVM_BUILD_CCACHE is now considered deprecated, so we should use cmake's compiler launcher variables. These variables have been supported since cmake 3.4, which is much older than LLVM's minimum required version, so we can wholesale replace LLVM_BUILD_CCACHE wirh thee variables, which will work for all LLVM releases. Link: https://discourse.llvm.org/t/llvm-ccache-build-is-deprecated/68431 Signed-off-by: Nathan Chancellor <nathan@kernel.org>
* origin/main: build-llvm.py: Fix setting LD fails due to inverted test-check (ClangBuiltLinux#228) This is not relevant for the rewrite, so this is just an empty merge to avoid conflicts. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Something I just noticed today Nathan, llvm and bintuils don't use the same args for providing a build dir - is this an opportunity to align the two? |
Sure thing, might as well when making breaking changes: a7edf39 |
…ild-folder' '-b' is the build folder shorthand in build-llvm.py. Use it for build-binutils.py as well, as people are more likely to change the build folder than the source being used. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
d17a20e
to
888565c
Compare
I have pushed away the |
This is an opinionated set of warnings from the full list that ruff supports. Link: https://beta.ruff.rs/docs/rules/ Signed-off-by: Nathan Chancellor <nathan@kernel.org>
This has been tastefully edited to omit using commas for function calls but for iterables, it makes sense. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
build-llvm.py:404:31: C400 [*] Unnecessary generator (rewrite as a `list` comprehension) Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Resolves ruff warning: tc_build/llvm.py:93:32: RUF005 [*] Consider `[self.tools.merge_fdata, *list(fdata_files)]` instead of concatenation Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
tc_build/llvm.py:232:9: SIM102 Use a single `if` statement instead of nested `if` statements Signed-off-by: Nathan Chancellor <nathan@kernel.org>
build-binutils.py:112:5: PLW2901 Outer for loop variable `target` overwritten by inner assignment target build-llvm.py:595:13: PLW2901 Outer for loop variable `pgo_target` overwritten by inner assignment target Signed-off-by: Nathan Chancellor <nathan@kernel.org>
As recently suggested by ruff: build-llvm.py:620:17: PLR5501 Consider using `elif` instead of `else` then `if` to remove one indentation level Signed-off-by: Nathan Chancellor <nathan@kernel.org>
This adds support for the ruff Python linter on top of the rewrite branch, which ensures that we get this right from the get go, rather than fixing on top. This is done as a separate series/merge to keep the history of the rewrite branch stable for testers. * rewrite-ruff: build-llvm.py: Refactor else + if into elif to reduce indentation build-*.py Fix new ruff warnings around iteration variables tc_build: llvm: Combine nested if statement tc_build: llvm: Ignore exception using contextlib.suppress() tc_build: llvm: Use unpacking instead of concatentation build-llvm.py: Use comprehension instead of constructor plus generator build-llvm.py: Use ternary expression for targets assignment tc-build: Run 'ruff --fix --select COM .' and adjust formatting for yapf Add a ruff configuration file Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
-Werror can cause kernel builds to fail, which is a poor experience when just building the kernel for profiling coverage, so it is disabled with 'KCFLAGS=-Wno-error'. Unfortunately, arch/powerpc has a separate configuration option to control building just that subdirectory with -Werror, which overrides the -Wno-error that was provided. Disable -Werror for powerpc using the special 'disable-werror.config' target, which merges the configuration to disable -Werror into the existing configuration, aligning powerpc with the global -Wno-error we apply for every other architecture. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
This matches our continuous integration and fixes building LLVM with PGO prior to 13.0.0. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
LLVM_VP_COUNTERS_PER_SITE does not exist in LLVM 11.x but the flags controlled by it are still needed when doing PGO with LLVM 11.x to avoid the same spew of warnings. Provide these flags ourselves via CMAKE_{C,CXX}_FLAGS. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
See the issue in the comments for more details. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
This gives builders flexibility with what exactly gets installed. Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Changing the default target triple potentially ties a toolchain to a specific operating system. Allow opting out of this by setting DISTRIBUTING=1 in the environment Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
import subprocess | ||
|
||
|
||
class Folders: |
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's not super common to have class names be plural. Having a class Folder, then having a list of instances of Folder objects with the member or variable identifier of folders would perhaps be more ergonomic.
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's not super common to have class names be plural.
Understandable.
Having a class Folder, then having a list of instances of Folder objects with the member or variable identifier of folders would perhaps be more ergonomic.
The whole point of the class is to keep the folders grouped together and easily identifiable. At that point, I might as well just turn this into a dictionary, right?
https://gist.github.com/nathanchance/06016b8ac2afcdd50e2fea6432136b33
I do not really have a strong opinion though.
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
I plan to merge this in its current state in a few hours, I think it has had enough time for review and testing. I am happy to address any follow up comments with new pull requests, should there be any. Please file issues for any bugs found after the merge for easy tracking. |
@nathanchance Will there be differences for users? Will the command line arguments be the same? |
@MajorP93 yes, there are some breaking changes, most of which you can read about in 8bc3594. You can see the command line arguments for each here to prepare for any changes you may have to make: Lines 14 to 67 in 0c1452e
Lines 26 to 399 in 0c1452e
You can see the changes that I had to make for my own personal wrappers here: |
Fixes the following trace back when using '--use-good-revision': Traceback (most recent call last): File ".../tc-build/./build-llvm.py", line 460, in <module> llvm_source.update(args.ref) File ".../tc-build/tc_build/llvm.py", line 541, in update if local_ref and local_ref.startswith('refs/heads/'): UnboundLocalError: local variable 'local_ref' referenced before assignment Fixes: 5cf3ec4 ("tc_build: llvm: Ignore exception using contextlib.suppress()") Signed-off-by: Nathan Chancellor <nathan@kernel.org>
@nathanchance Ok I see. Thanks for the explanation. |
This is currently a work in progress: I believe
build-llvm.py
is feature complete but I still need to portbuild-binutils.py
over and rewire the basic CI. Asbuild-llvm.py
is the main point of this repository, I wanted to push this for initial testing and review as soon as possible.There are quite a few breaking changes that I made in the pursuit of simplicity and maintainability. See the commit message of the rewrite commit for the full list, I do not want to be constantly editing the commit message and the message of this pull request as things pop up that need to be mentioned. The rewrite commit hash is currently 2fb5c1b but it may change throughout review.
See that same commit message for some of the reasoning behind this; I think this is much more maintainable as everything is clearly separated and compartmentalized now. I apologize that the rewrite is a single commit instead of a series, I hope it will not be a burden to review. I tried to keep everything easily separated so that each area was not overwhelming to understand.
I welcome any and all bug reports but please keep them here rather than in the issues tab until this is merged.
cc @ConchuOD @dileks @kees, as I know you use the script at times, so I would like to avoid breaking your use cases.