From e2518099a8e8352cd150a72b4ac11c8215493f2a Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Wed, 7 Feb 2018 13:29:02 -0800 Subject: [PATCH 1/4] Change corefx test process Instead of using the `/p:CoreCLROverridePath` switch when building corefx, to bring in coreclr, build corefx normally. This builds against the coreclr packages that corefx has already been validated against in the CI, so the build should always work. Then, copy over our built coreclr bits before testing. This method is expected to have better behavior in most breaking change scenarios. --- tests/scripts/run-corefx-tests.py | 56 ++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/tests/scripts/run-corefx-tests.py b/tests/scripts/run-corefx-tests.py index 246ec71eebbe..0671f9e8986d 100644 --- a/tests/scripts/run-corefx-tests.py +++ b/tests/scripts/run-corefx-tests.py @@ -29,6 +29,8 @@ # Globals ########################################################################## +testing = False + Corefx_url = 'https://github.com/dotnet/corefx.git' # This should be factored out of build.sh @@ -181,6 +183,29 @@ def log(message): print '[%s]: %s' % (sys.argv[0], message) +def update_target_files(source_dir, target_dir): + """ Copy any files in the source_dir to the target_dir, but only if they + already exist in target_dir. The copy is not recursive. The directories + must already exist. + Args: + source_dir (str): source directory path + target_dir (str): target directory path + Returns: + Nothing + """ + + global testing + assert os.path.isdir(source_dir) + assert os.path.isdir(target_dir) + + for source_filename in os.listdir(source_dir): + target_pathname = os.path.join(target_dir, source_filename) + if os.path.isfile(target_pathname): + source_pathname = os.path.join(source_dir, source_filename) + log('Copy: %s => %s' % (source_pathname, target_pathname)) + if not testing: + shutil.copy2(source_pathname, target_pathname) + ########################################################################## # Main ########################################################################## @@ -188,8 +213,7 @@ def log(message): def main(args): global Corefx_url global Unix_name_map - - testing = False + global testing arch, ci_arch, build_type, clr_root, fx_root, fx_branch, fx_commit, env_script, no_run_tests = validate_args( args) @@ -261,16 +285,40 @@ def main(args): config_args = '-Release -os:%s -buildArch:%s' % (clr_os, arch) # Run the primary (non-test) corefx build + # We used to pass the argument: + # -- /p:CoreCLROverridePath= + # which causes the corefx build to overwrite its built core_root with the binaries from + # the coreclr build. However, this often causes build failures which breaking changes are + # in progress (e.g., a breaking change is made in coreclr that has not yet had compensating + # changes made in the corefx repo). Instead, build corefx normally. This should always work + # since corefx is protected by a CI testing system. Then, overwrite the built corex + # runtime with the runtime built in the coreclr build. The result will be that perhaps + # some, hopefully few, corefx tests will fail, but the builds will never fail. command = ' '.join(('build.cmd' if Is_windows else './build.sh', - config_args, - '-- /p:CoreCLROverridePath=%s' % core_root)) + config_args)) log(command) returncode = 0 if testing else os.system(command) if returncode != 0: sys.exit(1) + # Override the built corefx runtime (which it picked up by copying from packages determined + # by its dependencies.props file). Note that we always build Release corefx. + # TODO: it might be cleaner to encapsulate the knowledge of how to do this in the + # corefx msbuild files somewhere. + + fx_runtime = os.path.join(fx_root, + 'bin', + 'testhost', + 'netcoreapp-%s-%s-%s' % (clr_os, 'Release', arch), + 'shared', + 'Microsoft.NETCore.App', + '9.9.9') + + log('Updating CoreCLR: %s => %s' % (core_root, fx_runtime)) + update_target_files(core_root, fx_runtime) + # Build the build-tests command line. if Is_windows: From 8d65d8b7b0fd359c7059fa444855620bc43cd0e0 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Wed, 7 Feb 2018 14:48:21 -0800 Subject: [PATCH 2/4] Update comments --- tests/scripts/run-corefx-tests.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/scripts/run-corefx-tests.py b/tests/scripts/run-corefx-tests.py index 0671f9e8986d..f083c782b8de 100644 --- a/tests/scripts/run-corefx-tests.py +++ b/tests/scripts/run-corefx-tests.py @@ -284,14 +284,15 @@ def main(args): config_args = '-Release -os:%s -buildArch:%s' % (clr_os, arch) - # Run the primary (non-test) corefx build - # We used to pass the argument: - # -- /p:CoreCLROverridePath= - # which causes the corefx build to overwrite its built core_root with the binaries from - # the coreclr build. However, this often causes build failures which breaking changes are + # Run the primary (non-test) corefx build. We previously passed the argument: + # + # /p:CoreCLROverridePath= + # + # which causes the corefx build to overwrite its built runtime with the binaries from + # the coreclr build. However, this often causes build failures when breaking changes are # in progress (e.g., a breaking change is made in coreclr that has not yet had compensating # changes made in the corefx repo). Instead, build corefx normally. This should always work - # since corefx is protected by a CI testing system. Then, overwrite the built corex + # since corefx is protected by a CI testing system. Then, overwrite the built corefx # runtime with the runtime built in the coreclr build. The result will be that perhaps # some, hopefully few, corefx tests will fail, but the builds will never fail. From 434445764db61b50c1d69e97d27fc4d3486e50d1 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Wed, 7 Feb 2018 16:23:56 -0800 Subject: [PATCH 3/4] Fix for altjit --- tests/scripts/run-corefx-tests.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/tests/scripts/run-corefx-tests.py b/tests/scripts/run-corefx-tests.py index f083c782b8de..9824c0619d51 100644 --- a/tests/scripts/run-corefx-tests.py +++ b/tests/scripts/run-corefx-tests.py @@ -183,10 +183,10 @@ def log(message): print '[%s]: %s' % (sys.argv[0], message) -def update_target_files(source_dir, target_dir): - """ Copy any files in the source_dir to the target_dir, but only if they - already exist in target_dir. The copy is not recursive. The directories - must already exist. +def copy_files(source_dir, target_dir): + """ Copy any files in the source_dir to the target_dir. + The copy is not recursive. + The directories must already exist. Args: source_dir (str): source directory path target_dir (str): target directory path @@ -199,12 +199,11 @@ def update_target_files(source_dir, target_dir): assert os.path.isdir(target_dir) for source_filename in os.listdir(source_dir): + source_pathname = os.path.join(source_dir, source_filename) target_pathname = os.path.join(target_dir, source_filename) - if os.path.isfile(target_pathname): - source_pathname = os.path.join(source_dir, source_filename) - log('Copy: %s => %s' % (source_pathname, target_pathname)) - if not testing: - shutil.copy2(source_pathname, target_pathname) + log('Copy: %s => %s' % (source_pathname, target_pathname)) + if not testing: + shutil.copy2(source_pathname, target_pathname) ########################################################################## # Main @@ -306,6 +305,8 @@ def main(args): # Override the built corefx runtime (which it picked up by copying from packages determined # by its dependencies.props file). Note that we always build Release corefx. + # We must copy all files, not just the files that already exist in the corefx runtime + # directory. This is required so we copy over all altjit compilers. # TODO: it might be cleaner to encapsulate the knowledge of how to do this in the # corefx msbuild files somewhere. @@ -318,7 +319,7 @@ def main(args): '9.9.9') log('Updating CoreCLR: %s => %s' % (core_root, fx_runtime)) - update_target_files(core_root, fx_runtime) + copy_files(core_root, fx_runtime) # Build the build-tests command line. From 621370424fff57db422c7ca05742c30649b80780 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Wed, 7 Feb 2018 20:12:46 -0800 Subject: [PATCH 4/4] Don't attempt to copy directories --- tests/scripts/run-corefx-tests.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/scripts/run-corefx-tests.py b/tests/scripts/run-corefx-tests.py index 9824c0619d51..7f261af7135a 100644 --- a/tests/scripts/run-corefx-tests.py +++ b/tests/scripts/run-corefx-tests.py @@ -200,10 +200,11 @@ def copy_files(source_dir, target_dir): for source_filename in os.listdir(source_dir): source_pathname = os.path.join(source_dir, source_filename) - target_pathname = os.path.join(target_dir, source_filename) - log('Copy: %s => %s' % (source_pathname, target_pathname)) - if not testing: - shutil.copy2(source_pathname, target_pathname) + if os.path.isfile(source_pathname): + target_pathname = os.path.join(target_dir, source_filename) + log('Copy: %s => %s' % (source_pathname, target_pathname)) + if not testing: + shutil.copy2(source_pathname, target_pathname) ########################################################################## # Main