Skip to content
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: Python rewrite #2

Merged
merged 85 commits into from
Apr 27, 2019

Conversation

nathanchance
Copy link
Member

As requested, the previous shell scripts have been rewriting in Python. Please feel free to let me know if there is something that could be done better, this is my first foray in Python after all :)

This passes my Docker testing (script here), which takes a Docker image, sets up an environment, and runs the scripts.

msfjarvis and others added 30 commits April 16, 2019 02:53
Signed-off-by: Harsh Shandilya <msfjarvis@gmail.com>
We are eventually going to have a build-binutils.py as well.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Incremental is supposed to be a boolean.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
These will be common functions we'll use across build-llvm.py and build-binutils.py.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
TODOs:

* Clang version checks
* Actually making the final variables accessible outside of the function

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
We want to avoid converting between PosixPath and str implicity because
prior to Python 3.6, many of the os methods wouldn't accept it,
resulting in a nice stacktrace (this one is from Debian's Python 3.5.3):

Traceback (most recent call last):
  File "./build-llvm.py", line 252, in <module>
    main()
  File "./build-llvm.py", line 245, in main
    fetch_llvm_binutils(root, not args.no_pull, args.branch)
  File "./build-llvm.py", line 156, in fetch_llvm_binutils
    subprocess.run(["git", "clone", "-b", branch,
"git://github.com/llvm/llvm-project", p], check=True)
  File "/usr/lib/python3.5/subprocess.py", line 383, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.5/subprocess.py", line 676, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.5/subprocess.py", line 1221, in _execute_child
    restore_signals, start_new_session, preexec_fn)
TypeError: Can't convert 'PosixPath' object to str implicitly

Pathlib has most of what we need and we can use the as_posix method to
get a stirng version of the path whenever we have to have it.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
krasCGQ added a commit to krasCGQ/scripts that referenced this pull request Apr 21, 2019
((Python is Snake))

Basically no point to keep maintaining Bash build-clang script
especially since I'm going to move to Zsh in the future.

**

Temporarily tracks nathanchance's python-bringup branch until scripts
are actually made a way to the actual repo, so expect some weird stuffs
to happen while they're on hold for revisions.

(ClangBuiltLinux/tc-build#2)

Signed-off-by: Albert I <krascgq@outlook.co.id>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
@ClangBuiltLinux ClangBuiltLinux deleted a comment from bwendling Apr 22, 2019
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
$ ./build-binutils.py -t x86_64 all
Traceback (most recent call last):
  File "./build-binutils.py", line 222, in <module>
    main()
  File "./build-binutils.py", line 215, in main
    print(create_targets(targets))
  File "./build-binutils.py", line 102, in create_targets
    targets_set.add(targets_dict[key])
KeyError: 'all'

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
… flags

This is handled a few rows down.

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Copy link
Member Author

@nathanchance nathanchance left a comment

Choose a reason for hiding this comment

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

Implemented rest of Miguel and Bill's suggestions

return parser.parse_args()


def create_targets(targets):
Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion, done: aff3e18

utils.py Outdated
print('\033[01;31m')
for x in range(0, len(string) + 6):
print("=", end="")
print("\n== " + string + " ==")
Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the clarification, done: c18e269

build-binutils.py Outdated Show resolved Hide resolved
utils.py Show resolved Hide resolved
build-binutils.py Outdated Show resolved Hide resolved
'CFLAGS=-O2 -march=native -mtune=native',
'CXXFLAGS=-O2 -march=native -mtune=native'
]
if "arm" in target or "aarch64" in target:
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, does this look good to you? 82a437e


# If we found a linker, we should use it
if env_vars.ld is not None:
defines['LLVM_USE_LINKER'] = env_vars.ld
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? I just tried this manually, and it seems that the check this does doesn't work when building clang w/ clang (ie when using -DCMAKE_CXX_COMPILER which seems like a deficiency with check_cxx_source_compiles not passing along cmake flags to try_compile.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, what does the print out of CC, CXX, and LD show at the beginning of the script? It works with all of the distributions I have checked in Docker.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but you're probably not setting -DCMAKE_C_COMPILER and -DCMAKE_CXX_COMPILER. Locally, I set these to Android's prebuilts in order to build clang with clang. I suppose this workflow is "unsupported" in cmake...

Copy link
Member Author

Choose a reason for hiding this comment

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

Those values are defined in this script on lines 318 and 320.

Copy link
Member

Choose a reason for hiding this comment

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

ah, thanks to @pirama-arumuga-nainar for pointing out -DLLVM_ENABLE_LLD=ON is a better approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only in the case of ld.lld. The user may be building with GCC which won't support that option or be using ld.gold.

-DLLVM_USE_LINKER should work no problem with clang, I'm curious what the fail looks like on your end.

Traceback (most recent call last):
  File "../build-llvm.py", line 435, in <module>
    main()
  File "../build-llvm.py", line 418, in main
    env_vars = EnvVars(*check_cc_ld_variables())
  File "../build-llvm.py", line 209, in check_cc_ld_variables
    if clang_version(cc) < 30900:
  File "../build-llvm.py", line 35, in clang_version
    return int(subprocess.check_output(["./clang-version.sh", cc]).decode())
  File "/usr/lib/python3.7/subprocess.py", line 395, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.7/subprocess.py", line 472, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.7/subprocess.py", line 775, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.7/subprocess.py", line 1522, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: './clang-version.sh': './clang-version.sh'

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Copy link
Member

@nickdesaulniers nickdesaulniers left a comment

Choose a reason for hiding this comment

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

LGTM great job Nathan

@nathanchance
Copy link
Member Author

Thanks Nick! I will merge this later today unless anyone has any further comments/objections :)

Copy link

@stephenhines stephenhines left a comment

Choose a reason for hiding this comment

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

Wow, everything looks great! I only had the 1 minor nit, but did look through the LLVM side of things a bit. In the future (TODO), the toolchain build should ideally be multi-stage (to make everything else more reproducible), but this is a huge improvement for everyone's productivity here. Thanks!


def host_is_target(target):
"""
Checks in the current target triple the same as the host?

Choose a reason for hiding this comment

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

nit: s/in/if

if machine in host_mapping.keys():
return host_mapping[machine]
else:
return machine

Choose a reason for hiding this comment

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

One more nitpick, this can be replaced with the one-liner

return host_mapping.get(machine, machine)

get(key[, default])
Return the value for key if key is in the dictionary, else default. If default is not given, it defaults to None, so that this method never raises a KeyError.

@nathanchance
Copy link
Member Author

Ack to both comments, I will push them later tonight.

@stephenhines just so that I am clear with the multistage build, that would be building LLVM/Clang then using that to build LLVM/Clang again? Were there any other constraints I should be aware of? For the record, I do a multistage build with ThinLTO for personal use so I could port that into here as a option for more optimization.

Signed-off-by: Harsh Shandilya <msfjarvis@gmail.com>
Signed-off-by: Harsh Shandilya <msfjarvis@gmail.com>
@stephenhines
Copy link

Yes, it's generally just using the newly built Clang to build the same sources again (but with the current Clang's optimizations and code generation). ThinLTO is also nice to add to a stage 2 build. It's not strictly necessary, but can be pretty nice if you are doing a lot of kernel builds with the same built toolchain.

@msfjarvis
Copy link
Member

Ack to both comments, I will push them later tonight.

I was on my PC so did the needful 👍

@nathanchance
Copy link
Member Author

@stephenhines thanks, I will work on that this weekend!

@MSF-Jarvis I appreciate it, I am running it through my last set of tests and then I will merge if they come back successful.

Traceback (most recent call last):
  File "./build-llvm.py", line 436, in <module>
    main()
  File "./build-llvm.py", line 419, in main
    env_vars = EnvVars(*check_cc_ld_variables(root_folder))
  File "./build-llvm.py", line 210, in check_cc_ld_variables
    if clang_version(cc, root_folder) < 30900:
  File "./build-llvm.py", line 36, in clang_version
    return int(subprocess.check_output(command).decode())
  File "/usr/lib/python3.5/subprocess.py", line 316, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.5/subprocess.py", line 383, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/usr/lib/python3.5/subprocess.py", line 676, in __init__
    restore_signals, start_new_session)
  File "/usr/lib/python3.5/subprocess.py", line 1205, in _execute_child
    executable = os.fsencode(executable)
  File "/usr/lib/python3.5/os.py", line 862, in fsencode
    raise TypeError("expect bytes or str, not %s" % type(filename).__name__)
TypeError: expect bytes or str, not PosixPath

Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
@nathanchance nathanchance merged commit ab729e2 into ClangBuiltLinux:master Apr 27, 2019
@nathanchance nathanchance deleted the python-bringup branch April 28, 2019 18:38
@jyizheng jyizheng mentioned this pull request Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants