-
Notifications
You must be signed in to change notification settings - Fork 70
Refine build tooling #327
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
base: master
Are you sure you want to change the base?
Refine build tooling #327
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,8 +29,8 @@ | |
|
|
||
| _python_version_ = '{}.{}'.format(sys.version_info.major, sys.version_info.minor) # should be formatted: 2.7, 3.5, 3.6, ... | ||
|
|
||
| _pybind11_version_ = 'aa304c9c7d725ffb9d10af08a3b34cb372307020' | ||
|
|
||
| #_pybind11_version_ = 'aa304c9c7d725ffb9d10af08a3b34cb372307020' | ||
| _pybind11_version_ = 'a2e59f0e7065404b44dfe92a28aca47ba1378dc4' | ||
lyskov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def execute(message, command_line, return_='status', until_successes=False, terminate_on_failure=True, silent=False, silence_output=False): | ||
| if not silent: print(message); print(command_line); sys.stdout.flush(); | ||
|
|
@@ -89,7 +89,7 @@ def get_cmake_compiler_options(compiler): | |
| return '' | ||
|
|
||
|
|
||
| def install_llvm_tool(name, source_location, prefix_root, debug, compiler, jobs, gcc_install_prefix, clean=True, llvm_version=None): | ||
| def install_llvm_tool(name, source_location, prefix_root, debug, compiler, jobs, gcc_install_prefix, clean=True, llvm_version=None, update_binder=False): | ||
| ''' Install and update (if needed) custom LLVM tool at given prefix (from config). | ||
| Return absolute path to executable on success and terminate with error on failure | ||
| ''' | ||
|
|
@@ -154,7 +154,7 @@ def install_llvm_tool(name, source_location, prefix_root, debug, compiler, jobs, | |
| if signature == disk_signature: | ||
| print('LLVM:{} + Binder install is detected at {}, skipping LLVM installation and Binder building procedures...\n'.format(llvm_version, build_dir)) | ||
|
|
||
| else: | ||
| elif not update_binder: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit odd, - if signature does not match then it is clear that Binder upgrade is needed. Could you please elaborate on the logic here? My initial impression for a new
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole point of this option is to not re-compile LLVM every time, which somehow happened for me each time I called the script after changing Binder code. So with this, LLVM will only be build if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm… it sounds like there is a disconnect here, please let me know if below is not what you are observing on your system: Note that I do like the idea of having |
||
| print('LLVM build detected, but config/binder version has changed, perfoming a clean rebuild...') | ||
| if os.path.isdir(build_dir): shutil.rmtree(build_dir) | ||
|
|
||
|
|
@@ -219,6 +219,13 @@ def install_llvm_tool(name, source_location, prefix_root, debug, compiler, jobs, | |
| if not os.path.isfile(cmake_lists): | ||
| with open(cmake_lists, 'w') as f: f.write(tool_build_line + '\n') | ||
|
|
||
| # We are building from scratch, so we want to build Binder as well. | ||
| update_binder = True | ||
|
|
||
| # The above takes care of LLVM being built and ready. However, we might want to update just | ||
| # the build for Binder itself, without having to recompile all of LLVM, for instance during dev. | ||
| # Thus, here, we can re-run cmake to just build the binder binary again. | ||
| if update_binder: | ||
| config = '-DCMAKE_BUILD_TYPE={build_type}'.format(build_type='Debug' if debug else 'Release') | ||
| config += get_cmake_compiler_options(compiler) | ||
|
|
||
|
|
@@ -278,13 +285,14 @@ def main(args): | |
| parser.add_argument('--llvm-version', default=None, choices=['6.0.1', '13.0.0', '14.0.5'], help='Manually specify LLVM version to install') | ||
| parser.add_argument('--annotate-includes', action="store_true", help='Annotate includes in generated source files') | ||
| parser.add_argument('--trace', action="store_true", help='Binder will add trace output to to generated source files') | ||
| parser.add_argument('--update-binder', action="store_true", help='Recompile only the Binder binary; useful when working on the Binder code') | ||
|
|
||
| global Options | ||
| Options = parser.parse_args() | ||
|
|
||
| source_path = os.path.abspath('.') | ||
|
|
||
| if not Options.binder: Options.binder = install_llvm_tool('binder', source_path+'/source', source_path + '/build', debug=Options.binder_debug, compiler=Options.compiler, jobs=Options.jobs, gcc_install_prefix=None, llvm_version=Options.llvm_version) | ||
| if not Options.binder: Options.binder = install_llvm_tool('binder', source_path+'/source', source_path + '/build', debug=Options.binder_debug, compiler=Options.compiler, jobs=Options.jobs, gcc_install_prefix=None, llvm_version=Options.llvm_version, update_binder=Options.update_binder) | ||
|
|
||
| if not Options.pybind11: Options.pybind11 = install_pybind11(source_path + '/build') | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,14 +131,17 @@ def run_test(test_path, build_dir, pyenv): | |
| command_line = 'diff {ref} {new}'.format(**vars()) | ||
|
|
||
| remover_absolute_paths(new) | ||
| r = execute('Comparing results for test {}...'.format(test), command_line, terminate_on_failure=not Options.accept); | ||
| r = execute('Comparing results for test {}...'.format(test), command_line, terminate_on_failure=not (Options.accept or Options.accept_all )); | ||
|
|
||
| not_binded = [l for l in open(new).read().split('\n') if 'not_binded' in l] | ||
| if not_binded: print('{}\n"not_binded" string found in results for test {}!!!\n'.format('\n'.join(not_binded), test)); sys.exit(1) | ||
|
|
||
| if r and Options.accept: | ||
| a = input( 'Accept new results from test {test} as reference? [Y/n] '.format(test=test) ) | ||
| if a in ['', 'y', 'yes']: shutil.copyfile(new, ref) | ||
| if r: | ||
| if Options.accept and not Options.accept_all: | ||
| a = input( 'Accept new results from test {test} as reference? [Y/n] '.format(test=test) ) | ||
| if a in ['', 'y', 'yes']: shutil.copyfile(new, ref) | ||
| if Options.accept_all: | ||
| shutil.copyfile(new, ref) | ||
|
|
||
|
|
||
|
|
||
|
|
@@ -151,7 +154,8 @@ def main(): | |
|
|
||
| parser.add_argument('--pybind11', default='', help='Path to pybind11 source tree') | ||
|
|
||
| parser.add_argument("--accept", action="store_true", help="Run tests and accept new tests results as reference") | ||
| parser.add_argument("--accept", action="store_true", help="Run tests and ask to accept new tests results as reference") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we on this, probably "optional" since it is not directly related to new code: name of this option now clearly misleading, especially with introduction of
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with changing compatibility in this case (I do not think this option is used in automation), so please feel to rename it. Thanks, |
||
| parser.add_argument("--accept-all", action="store_true", help="Run tests and automatically accept all new tests results as reference") | ||
| parser.add_argument("--annotate", action="store_true", help="Run Binder with extra annotation options") | ||
|
|
||
| parser.add_argument('args', nargs=argparse.REMAINDER, help='Optional: list of tests to run') | ||
|
|
@@ -162,6 +166,7 @@ def main(): | |
| test_source_dir = os.path.dirname( os.path.abspath(__file__) ) | ||
|
|
||
| tests = Options.args or [ t for t in sorted( get_test_files(test_source_dir) ) if 'T61.smart_holder.hpp' not in t ] | ||
| print('Found {ntests} test cases'.format( ntests=len(tests) )) | ||
|
|
||
| build_dir = test_source_dir + '/build' | ||
| if os.path.isdir(build_dir): print('Removing old test dir {0} ...'.format(build_dir)); shutil.rmtree(build_dir) # remove old dir if any | ||
|
|
||
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.
Could you please use something like
ubuntu-24.04instead of latest? We do not want this to suddenly start failing if/when GitHub update latest-runner image.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.
Sure, will do!