From f236c65a1bb05ce788970c9be20cc61770c9dbc7 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Tue, 28 Aug 2018 16:38:31 +0200 Subject: [PATCH 1/3] Windows, build-runfiles: Implement build-runfiles for Windows Working towards: https://github.com/bazelbuild/bazel/issues/5807 Change-Id: Iae10ff439bb3e8513c44ebfc4c174409623991d9 --- src/main/cpp/util/BUILD | 1 + .../analysis/config/BuildConfiguration.java | 4 - src/main/tools/BUILD | 1 + src/main/tools/build-runfiles-windows.cc | 391 +++++++++++++++++- src/test/py/bazel/runfiles_test.py | 10 - src/test/shell/integration/BUILD | 7 +- src/test/shell/integration/runfiles_test.sh | 188 +++++++-- 7 files changed, 551 insertions(+), 51 deletions(-) diff --git a/src/main/cpp/util/BUILD b/src/main/cpp/util/BUILD index 9df4e558d8f472..bc799aba338af7 100644 --- a/src/main/cpp/util/BUILD +++ b/src/main/cpp/util/BUILD @@ -55,6 +55,7 @@ cc_library( visibility = [ ":ijar", "//src/test/cpp/util:__pkg__", + "//src/main/tools:__pkg__", "//src/tools/launcher:__subpackages__", "//src/tools/singlejar:__pkg__", "//third_party/def_parser:__pkg__", diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java index 7590d6aa696d28..2b58187dd7005d 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfiguration.java @@ -1167,10 +1167,6 @@ public void reportInvalidOptions(EventHandler reporter) { fragment.reportInvalidOptions(reporter, this.buildOptions); } - if (OS.getCurrent() == OS.WINDOWS && runfilesEnabled()) { - reporter.handle(Event.error("building runfiles is not supported on Windows")); - } - if (options.outputDirectoryName != null) { reporter.handle(Event.error( "The internal '--output directory name' option cannot be used on the command line")); diff --git a/src/main/tools/BUILD b/src/main/tools/BUILD index e67d67baaa6953..8362c0c0b6dd22 100644 --- a/src/main/tools/BUILD +++ b/src/main/tools/BUILD @@ -48,6 +48,7 @@ cc_binary( "//src/conditions:windows": ["build-runfiles-windows.cc"], "//conditions:default": ["build-runfiles.cc"], }), + deps = ["//src/main/cpp/util:filesystem"], ) cc_binary( diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index ea4eed189aab26..14ac4bfb9e9d59 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/src/main/tools/build-runfiles-windows.cc @@ -12,16 +12,389 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include #include +#include +#include +#include +#include +#include -int main(int argc, char** argv) { - // TODO(bazel-team): decide whether we need build-runfiles at all on Windows. - // Implement this program if so; make sure we don't run it on Windows if not. - std::cout << "ERROR: build-runfiles is not (yet?) implemented on Windows." - << std::endl - << "Called with args:" << std::endl; - for (int i = 0; i < argc; ++i) { - std::cout << "argv[" << i << "]=(" << argv[i] << ")" << std::endl; +#include "src/main/cpp/util/path_platform.h" +#include "src/main/cpp/util/file_platform.h" +#include "src/main/cpp/util/strings.h" + +using std::ifstream; +using std::unordered_map; +using std::string; +using std::wstring; +using std::stringstream; + +#define ManifestFileMap unordered_map + +#ifndef SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE +#define SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE 0x2 +#endif + +#ifndef SYMBOLIC_LINK_FLAG_DIRECTORY +#define SYMBOLIC_LINK_FLAG_DIRECTORY 0x1 +#endif + +const wchar_t *manifest_filename; +const wchar_t *runfiles_base_dir; + +string GetLastErrorString() { + DWORD last_error = GetLastError(); + if (last_error == 0) { + return string(); + } + + char* message_buffer; + size_t size = FormatMessageA( + FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | + FORMAT_MESSAGE_IGNORE_INSERTS, + NULL, last_error, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), + (LPSTR)&message_buffer, 0, NULL); + + stringstream result; + result << "(error: " << last_error << "): " << message_buffer; + LocalFree(message_buffer); + return result.str(); +} + +void die(const wchar_t* format, ...) { + va_list ap; + va_start(ap, format); + fputws(L"build-runfiles error: ", stderr); + vfwprintf(stderr, format, ap); + va_end(ap); + fputwc(L'\n', stderr); + fputws(L"manifest file name: ", stderr); + fputws(manifest_filename, stderr); + fputwc(L'\n', stderr); + fputws(L"runfiles base directory: ", stderr); + fputws(runfiles_base_dir, stderr); + fputwc(L'\n', stderr); + exit(1); +} + +wstring AsAbsoluteWindowsPath(const wchar_t* path) { + wstring wpath; + string error; + if (!blaze_util::AsAbsoluteWindowsPath(path, &wpath, &error)) { + die(L"Couldn't convert %s to absolute Windows path: %hs", path, + error.c_str()); + } + return wpath; +} + +bool DoesDirectoryPathExist(const wchar_t* path) { + DWORD dwAttrib = GetFileAttributesW(AsAbsoluteWindowsPath(path).c_str()); + + return (dwAttrib != INVALID_FILE_ATTRIBUTES && + (dwAttrib & FILE_ATTRIBUTE_DIRECTORY)); +} + +wstring GetParentDirFromPath(const wstring& path) { + return path.substr(0, path.find_last_of(L"\\/")); +} + +inline wstring Trim(const wstring& str){ + wstring result = str; + result.erase(0, result.find_first_not_of(' ')); + result.erase(result.find_last_not_of(' ') + 1); + return result; +} + +class RunfilesCreator { + public: + explicit RunfilesCreator(const wstring &manifest_path, + const wstring &runfiles_output_base) + : manifest_path_(manifest_path), + runfiles_output_base_(runfiles_output_base) { + SetupOutputBase(); + if (!SetCurrentDirectoryW(runfiles_output_base_.c_str())) { + die(L"SetCurrentDirectoryW failed (%s): %hs", + runfiles_output_base_.c_str(), GetLastErrorString().c_str()); + } + } + + void ReadManifest(bool allow_relative, bool use_metadata) { + ifstream manifest_file( + AsAbsoluteWindowsPath(manifest_path_.c_str()).c_str()); + + if (!manifest_file) { + die(L"Couldn't open MANIFEST file: %s", manifest_path_.c_str()); + } + + string line; + int lineno = 0; + while (getline(manifest_file, line)) { + lineno++; + // Skip metadata lines. They are used solely for + // dependency checking. + if (use_metadata && lineno % 2 == 0) { + continue; + } + + size_t space_pos = line.find_first_of(' '); + wstring wline = blaze_util::CstringToWstring(line); + wstring link, target; + if (space_pos == string::npos) { + link = wline; + target = wstring(); + } else { + link = wline.substr(0, space_pos); + target = wline.substr(space_pos + 1); + } + + // Removing leading and trailing spaces + link = Trim(link); + target = Trim(target); + + if (!allow_relative && !target.empty() + && !blaze_util::IsAbsolute(target)) { + die(L"Target cannot be relative path: %hs", line.c_str()); + } + + link = AsAbsoluteWindowsPath(link.c_str()); + + manifest_file_map.insert(make_pair(link, target)); + } + manifest_file.close(); + } + + void CreateRunfiles() { + bool symlink_needs_privilege = + DoesCreatingSymlinkNeedAdminPrivilege(runfiles_output_base_); + ScanTreeAndPrune(runfiles_output_base_); + CreateFiles(symlink_needs_privilege); + CopyManifestFile(); + } + + private: + void SetupOutputBase() { + if (!DoesDirectoryPathExist(runfiles_output_base_.c_str())) { + MakeDirectoriesOrDie(runfiles_output_base_); + } } - return 1; + + void MakeDirectoriesOrDie(const wstring& path) { + if (!blaze_util::MakeDirectoriesW(path, 0755)) { + die(L"MakeDirectoriesW failed (%s): %hs", + path.c_str(), GetLastErrorString().c_str()); + } + } + + void RemoveDirectoryOrDie(const wstring& path) { + if (!RemoveDirectoryW(path.c_str())) { + die(L"RemoveDirectoryW failed (%s): %hs", GetLastErrorString().c_str()); + } + } + + void DeleteFileOrDie(const wstring& path) { + SetFileAttributesW(path.c_str(), + GetFileAttributesW(path.c_str()) & ~FILE_ATTRIBUTE_READONLY); + if (!DeleteFileW(path.c_str())) { + die(L"DeleteFileW failed (%s): %hs", + path.c_str(), GetLastErrorString().c_str()); + } + } + + bool DoesCreatingSymlinkNeedAdminPrivilege(const wstring& runfiles_base_dir) { + wstring dummy_link = runfiles_base_dir + L"\\dummy_link"; + wstring dummy_target = runfiles_base_dir + L"\\dummy_target"; + + // Try creating symlink without admin privilege. + if (CreateSymbolicLinkW(dummy_link.c_str(), dummy_target.c_str(), + SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE)) { + DeleteFileOrDie(dummy_link); + return false; + } + + // Try creating symlink with admin privilege + if (CreateSymbolicLinkW(dummy_link.c_str(), dummy_target.c_str(), 0)) { + DeleteFileOrDie(dummy_link); + return true; + } + + // If we couldn't create symlink, print out an error message and exit. + if (GetLastError() == ERROR_PRIVILEGE_NOT_HELD) { + die(L"CreateSymbolicLinkW failed:\n%hs\n", + "Bazel needs to create symlink for building runfiles tree.\n" + "Creating symlink on Windows requires either of the following:\n" + " 1. Program is running with elevated privileges (Admin rights).\n" + " 2. The system version is Windows 10 Creators Update (1703) or later and " + "developer mode is enabled.", + GetLastErrorString().c_str()); + } else { + die(L"CreateSymbolicLinkW failed: %hs", GetLastErrorString().c_str()); + } + + return true; + } + + // This function scan the current directory, remove all files/symlinks/directories that + // are not presented in manifest file. + // If a symlink already exists and points to the correct target, this function erases + // its entry from manifest_file_map, so that we won't recreate it. + void ScanTreeAndPrune(const wstring& path) { + static const wstring kDot(L"."); + static const wstring kDotDot(L".."); + + WIN32_FIND_DATAW metadata; + HANDLE handle = ::FindFirstFileW((path + L"\\*").c_str(), &metadata); + if (handle == INVALID_HANDLE_VALUE) { + return; // directory does not exist or is empty + } + + do { + if (kDot != metadata.cFileName && kDotDot != metadata.cFileName) { + wstring subpath = path + L"\\" + metadata.cFileName; + bool is_dir = + (metadata.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0; + bool is_symlink = + (metadata.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) != 0; + if (is_symlink) { + wstring target; + if (!blaze_util::ReadSymlinkW(subpath, &target)) { + die(L"ReadSymlinkW failed (%s): %hs", + subpath.c_str(), GetLastErrorString().c_str()); + }; + + subpath = AsAbsoluteWindowsPath(subpath.c_str()); + target = AsAbsoluteWindowsPath(target.c_str()); + ManifestFileMap::iterator expected_target = + manifest_file_map.find(subpath); + + if (expected_target == manifest_file_map.end() + || expected_target->second.empty() + || target != AsAbsoluteWindowsPath(expected_target->second.c_str()) + || blaze_util::IsDirectoryW(target) != is_dir) { + if (is_dir) { + RemoveDirectoryOrDie(subpath); + } else { + DeleteFileOrDie(subpath); + } + } else { + manifest_file_map.erase(expected_target); + } + } else { + if (is_dir) { + ScanTreeAndPrune(subpath); + // If the directory is empty, then we remove the directory. + // Otherwise RemoveDirectory will fail with ERROR_DIR_NOT_EMPTY, + // which we can just ignore. + if (!RemoveDirectoryW(subpath.c_str()) + && GetLastError() != ERROR_DIR_NOT_EMPTY) { + die(L"RemoveDirectoryW failed (%s): %hs", + subpath.c_str(), GetLastErrorString().c_str()); + } + } else { + DeleteFileOrDie(subpath); + } + } + } + } while (::FindNextFileW(handle, &metadata)); + ::FindClose(handle); + } + + void CreateFiles(bool creating_symlink_needs_admin_privilege) { + DWORD privilege_flag = creating_symlink_needs_admin_privilege + ? 0 + : SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE; + + for (ManifestFileMap::const_iterator it = manifest_file_map.begin(); + it != manifest_file_map.end(); ++it) { + + // Ensure the parent directory exists + wstring parent_dir = GetParentDirFromPath(it->first); + if (!DoesDirectoryPathExist(parent_dir.c_str())) { + MakeDirectoriesOrDie(parent_dir); + } + + if (it->second.empty()) { + // Create an empty file + HANDLE h = CreateFileW(it->first.c_str(), // name of the file + GENERIC_WRITE, // open for writing + 0, // sharing mode, none in this case + 0, // use default security descriptor + CREATE_ALWAYS, // overwrite if exists + FILE_ATTRIBUTE_NORMAL, + 0); + if (h) { + CloseHandle(h); + } else { + die(L"CreateFileW failed (%s): %hs", + it->first.c_str(), GetLastErrorString().c_str()); + } + } else { + DWORD create_dir = 0; + if (blaze_util::IsDirectoryW(it->second.c_str())) { + create_dir = SYMBOLIC_LINK_FLAG_DIRECTORY; + } + if (!CreateSymbolicLinkW(it->first.c_str(), + it->second.c_str(), + privilege_flag | create_dir)) { + die(L"CreateSymbolicLinkW failed (%s -> %s): %hs", + it->first.c_str(), it->second.c_str(), + GetLastErrorString().c_str()); + } + } + } + } + + void CopyManifestFile() { + wstring new_manifest_file = runfiles_output_base_ + L"\\MANIFEST"; + if (!CopyFileW(manifest_path_.c_str(), + new_manifest_file.c_str(), + /*bFailIfExists=*/ false + )) { + die(L"CopyFileW failed (%s -> %s): %hs", + manifest_path_.c_str(), new_manifest_file.c_str(), + GetLastErrorString().c_str()); + } + } + + private: + wstring manifest_path_; + wstring runfiles_output_base_; + ManifestFileMap manifest_file_map; +}; + +int wmain(int argc, wchar_t** argv) { + argc--; argv++; + bool allow_relative = false; + bool use_metadata = false; + + while (argc >= 1) { + if (wcscmp(argv[0], L"--allow_relative") == 0) { + allow_relative = true; + argc--; argv++; + } else if (wcscmp(argv[0], L"--use_metadata") == 0) { + use_metadata = true; + argc--; argv++; + } else { + break; + } + } + + if (argc != 2) { + fprintf(stderr, "usage: [--allow_relative] [--use_metadata] " + "INPUT RUNFILES\n"); + return 1; + } + + manifest_filename = argv[0]; + runfiles_base_dir = argv[1]; + + wstring manifest_absolute_path = AsAbsoluteWindowsPath(manifest_filename); + wstring output_base_absolute_path = AsAbsoluteWindowsPath(runfiles_base_dir); + + RunfilesCreator runfiles_creator(manifest_absolute_path, + output_base_absolute_path); + runfiles_creator.ReadManifest(allow_relative, use_metadata); + runfiles_creator.CreateRunfiles(); + + return 0; } diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py index 50506f63918e8a..36aba0d7678642 100644 --- a/src/test/py/bazel/runfiles_test.py +++ b/src/test/py/bazel/runfiles_test.py @@ -21,16 +21,6 @@ class RunfilesTest(test_base.TestBase): - def testAttemptToBuildRunfilesOnWindows(self): - if not self.IsWindows(): - self.skipTest("only applicable to Windows") - self.ScratchFile("WORKSPACE") - exit_code, _, stderr = self.RunBazel( - ["build", "--experimental_enable_runfiles"]) - self.assertNotEqual(exit_code, 0) - self.assertIn("building runfiles is not supported on Windows", - "\n".join(stderr)) - def _AssertRunfilesLibraryInBazelToolsRepo(self, family, lang_name): for s, t, exe in [("WORKSPACE.mock", "WORKSPACE", False), ("foo/BUILD.mock", "foo/BUILD", diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD index 5501058918bb29..efdde9ec93af03 100644 --- a/src/test/shell/integration/BUILD +++ b/src/test/shell/integration/BUILD @@ -29,11 +29,12 @@ sh_test( name = "runfiles_test", size = "medium", srcs = ["runfiles_test.sh"], - data = [":test-deps"], + data = [ + ":test-deps", + "@bazel_tools//tools/bash/runfiles", + ], tags = [ "local", - # no_windows: this test exercises the symlink-based runfiles tree. - "no_windows", ], ) diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index cb2e88627e0438..12a817361a8086 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -16,18 +16,64 @@ # # An end-to-end test that Bazel produces runfiles trees as expected. -# Load the test setup defined in the parent directory -CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -source "${CURRENT_DIR}/../integration_test_setup.sh" \ +# --- begin runfiles.bash initialization --- +# Copy-pasted from Bazel's Bash runfiles library (tools/bash/runfiles/runfiles.bash). +set -euo pipefail +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + +source "$(rlocation "io_bazel/src/test/shell/integration_test_setup.sh")" \ || { echo "integration_test_setup.sh not found!" >&2; exit 1; } +case "$(uname -s | tr [:upper:] [:lower:])" in +msys*|mingw*|cygwin*) + declare -r is_windows=true + ;; +*) + declare -r is_windows=false + ;; +esac + +if "$is_windows"; then + export MSYS_NO_PATHCONV=1 + export MSYS2_ARG_CONV_EXCL="*" +fi + #### SETUP ############################################################# set -e function set_up() { - mkdir -p pkg - cd pkg + if "$is_windows"; then + export EXT=".exe" + export EXTRA_BUILD_FLAGS="--experimental_enable_runfiles --build_python_zip=0" + else + export EXT="" + export EXTRA_BUILD_FLAGS="" + fi +} + +function create_pkg() { + local -r pkg=$1 + mkdir -p $pkg + cd $pkg mkdir -p a/b c/d e/f/g x/y touch py.py a/b/no_module.py c/d/one_module.py c/__init__.py e/f/g/ignored.py x/y/z.sh @@ -40,7 +86,9 @@ function set_up() { #### TESTS ############################################################# function test_hidden() { - cat > pkg/BUILD << EOF + local -r pkg=$FUNCNAME + create_pkg $pkg + cat > $pkg/BUILD << EOF py_binary(name = "py", srcs = [ "py.py" ], data = [ "e/f", @@ -49,19 +97,21 @@ genrule(name = "hidden", outs = [ "e/f/g/hidden.py" ], cmd = "touch \$@") EOF - bazel build pkg:py >&$TEST_log 2>&1 || fail "build failed" + bazel build $pkg:py $EXTRA_BUILD_FLAGS >&$TEST_log 2>&1 || fail "build failed" # we get a warning that hidden.py is inaccessible - expect_log_once "pkg/e/f/g/hidden.py obscured by pkg/e/f " + expect_log_once "${pkg}/e/f/g/hidden.py obscured by ${pkg}/e/f " } function test_foo_runfiles() { + local -r pkg=$FUNCNAME + create_pkg $pkg cat > BUILD << EOF py_library(name = "root", srcs = ["__init__.py"], visibility = ["//visibility:public"]) EOF -cat > pkg/BUILD << EOF +cat > $pkg/BUILD << EOF sh_binary(name = "foo", srcs = [ "x/y/z.sh" ], data = [ ":py", @@ -74,9 +124,10 @@ py_binary(name = "py", "e/f/g/ignored.py" ], deps = ["//:root"]) EOF - bazel build pkg:foo >&$TEST_log || fail "build failed" + bazel build $pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed" + workspace_root=$PWD - cd ${PRODUCT_NAME}-bin/pkg/foo.runfiles + cd ${PRODUCT_NAME}-bin/$pkg/foo${EXT}.runfiles # workaround until we use assert/fail macros in the tests below touch $TEST_TMPDIR/__fail @@ -88,10 +139,10 @@ EOF cd ${WORKSPACE_NAME} # these are real directories - test \! -L pkg - test -d pkg + test \! -L $pkg + test -d $pkg - cd pkg + cd $pkg test \! -L a test -d a test \! -L a/b @@ -133,15 +184,102 @@ EOF # that accounts for everything cd ../.. - assert_equals 9 $(find ${WORKSPACE_NAME} -type l | wc -l) - assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) - assert_equals 9 $(find ${WORKSPACE_NAME} -type d | wc -l) - assert_equals 22 $(find ${WORKSPACE_NAME} | wc -l) - assert_equals 13 $(wc -l < MANIFEST) + if "$is_windows"; then + assert_equals 10 $(find ${WORKSPACE_NAME} -type l | wc -l) + assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) + assert_equals 9 $(find ${WORKSPACE_NAME} -type d | wc -l) + assert_equals 23 $(find ${WORKSPACE_NAME} | wc -l) + assert_equals 14 $(wc -l < MANIFEST) + else + assert_equals 9 $(find ${WORKSPACE_NAME} -type l | wc -l) + assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) + assert_equals 9 $(find ${WORKSPACE_NAME} -type d | wc -l) + assert_equals 22 $(find ${WORKSPACE_NAME} | wc -l) + assert_equals 13 $(wc -l < MANIFEST) + fi + + for i in $(find ${WORKSPACE_NAME} \! -type d); do + if readlink "$i" > /dev/null; then + if "$is_windows"; then + echo "$i $(cygpath -m $(readlink "$i"))" >> ${TEST_TMPDIR}/MANIFEST2 + else + echo "$i $(readlink "$i")" >> ${TEST_TMPDIR}/MANIFEST2 + fi + else + echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 + fi + done + sort MANIFEST > ${TEST_TMPDIR}/MANIFEST_sorted + sort ${TEST_TMPDIR}/MANIFEST2 > ${TEST_TMPDIR}/MANIFEST2_sorted + diff -u ${TEST_TMPDIR}/MANIFEST_sorted ${TEST_TMPDIR}/MANIFEST2_sorted + + # Rebuild the same target with a new dependency. + cd "$workspace_root" +cat > $pkg/BUILD << EOF +sh_binary(name = "foo", + srcs = [ "x/y/z.sh" ], + data = [ "e/f" ]) +EOF + bazel build $pkg:foo $EXTRA_BUILD_FLAGS >&$TEST_log || fail "build failed" + + cd ${PRODUCT_NAME}-bin/$pkg/foo${EXT}.runfiles + + # workaround until we use assert/fail macros in the tests below + touch $TEST_TMPDIR/__fail + + # output manifest exists and is non-empty + test -f MANIFEST + test -s MANIFEST + + cd ${WORKSPACE_NAME} + + # these are real directories + test \! -L $pkg + test -d $pkg + + # these directory should not exist anymore + test \! -e a + test \! -e c + + cd $pkg + test \! -L e + test -d e + test \! -L x + test -d x + test \! -L x/y + test -d x/y + + # these are symlinks to the source tree + test -L foo + test -L x/y/z.sh + test -L e/f + test -d e/f + + # that accounts for everything + cd ../.. + if "$is_windows"; then + assert_equals 4 $(find ${WORKSPACE_NAME} -type l | wc -l) + assert_equals 0 $(find ${WORKSPACE_NAME} -type f | wc -l) + assert_equals 5 $(find ${WORKSPACE_NAME} -type d | wc -l) + assert_equals 9 $(find ${WORKSPACE_NAME} | wc -l) + assert_equals 4 $(wc -l < MANIFEST) + else + assert_equals 3 $(find ${WORKSPACE_NAME} -type l | wc -l) + assert_equals 0 $(find ${WORKSPACE_NAME} -type f | wc -l) + assert_equals 5 $(find ${WORKSPACE_NAME} -type d | wc -l) + assert_equals 8 $(find ${WORKSPACE_NAME} | wc -l) + assert_equals 3 $(wc -l < MANIFEST) + fi + rm -f ${TEST_TMPDIR}/MANIFEST + rm -f ${TEST_TMPDIR}/MANIFEST2 for i in $(find ${WORKSPACE_NAME} \! -type d); do if readlink "$i" > /dev/null; then - echo "$i $(readlink "$i")" >> ${TEST_TMPDIR}/MANIFEST2 + if "$is_windows"; then + echo "$i $(cygpath -m $(readlink "$i"))" >> ${TEST_TMPDIR}/MANIFEST2 + else + echo "$i $(readlink "$i")" >> ${TEST_TMPDIR}/MANIFEST2 + fi else echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 fi @@ -166,15 +304,15 @@ EOF cat > thing.cc < $TEST_log || fail "Build failed" - [[ -d ${PRODUCT_NAME}-bin/thing.runfiles/foo ]] || fail "foo not found" + bazel build //:thing $EXTRA_BUILD_FLAGS &> $TEST_log || fail "Build failed" + [[ -d ${PRODUCT_NAME}-bin/thing${EXT}.runfiles/foo ]] || fail "foo not found" cat > WORKSPACE < $TEST_log || fail "Build failed" - [[ -d ${PRODUCT_NAME}-bin/thing.runfiles/bar ]] || fail "bar not found" - [[ ! -d ${PRODUCT_NAME}-bin/thing.runfiles/foo ]] \ + bazel build //:thing $EXTRA_BUILD_FLAGS &> $TEST_log || fail "Build failed" + [[ -d ${PRODUCT_NAME}-bin/thing${EXT}.runfiles/bar ]] || fail "bar not found" + [[ ! -d ${PRODUCT_NAME}-bin/thing${EXT}.runfiles/foo ]] \ || fail "Old foo still found" } From 30f4ec34245a59810c6a083bfd78a4dab88623ec Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Thu, 30 Aug 2018 11:26:04 +0200 Subject: [PATCH 2/3] Addressing review comments Change-Id: If02139f98264ba9d282d29c405cb7eaedf4797c0 --- src/main/tools/build-runfiles-windows.cc | 49 ++++++++++----------- src/test/shell/integration/runfiles_test.sh | 37 +++++++--------- 2 files changed, 41 insertions(+), 45 deletions(-) diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index 14ac4bfb9e9d59..9f82e536b937a7 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/src/main/tools/build-runfiles-windows.cc @@ -30,8 +30,6 @@ using std::string; using std::wstring; using std::stringstream; -#define ManifestFileMap unordered_map - #ifndef SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE #define SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE 0x2 #endif @@ -40,6 +38,8 @@ using std::stringstream; #define SYMBOLIC_LINK_FLAG_DIRECTORY 0x1 #endif +namespace { + const wchar_t *manifest_filename; const wchar_t *runfiles_base_dir; @@ -99,16 +99,18 @@ wstring GetParentDirFromPath(const wstring& path) { return path.substr(0, path.find_last_of(L"\\/")); } -inline wstring Trim(const wstring& str){ - wstring result = str; - result.erase(0, result.find_first_not_of(' ')); - result.erase(result.find_last_not_of(' ') + 1); - return result; +inline void Trim(wstring& str){ + str.erase(0, str.find_first_not_of(' ')); + str.erase(str.find_last_not_of(' ') + 1); } +} // namespace + class RunfilesCreator { + typedef std::unordered_map ManifestFileMap; + public: - explicit RunfilesCreator(const wstring &manifest_path, + RunfilesCreator(const wstring &manifest_path, const wstring &runfiles_output_base) : manifest_path_(manifest_path), runfiles_output_base_(runfiles_output_base) { @@ -149,8 +151,8 @@ class RunfilesCreator { } // Removing leading and trailing spaces - link = Trim(link); - target = Trim(target); + Trim(link); + Trim(target); if (!allow_relative && !target.empty() && !blaze_util::IsAbsolute(target)) { @@ -161,7 +163,6 @@ class RunfilesCreator { manifest_file_map.insert(make_pair(link, target)); } - manifest_file.close(); } void CreateRunfiles() { @@ -304,40 +305,38 @@ class RunfilesCreator { ? 0 : SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE; - for (ManifestFileMap::const_iterator it = manifest_file_map.begin(); - it != manifest_file_map.end(); ++it) { - + for (const auto &it : manifest_file_map) { // Ensure the parent directory exists - wstring parent_dir = GetParentDirFromPath(it->first); + wstring parent_dir = GetParentDirFromPath(it.first); if (!DoesDirectoryPathExist(parent_dir.c_str())) { MakeDirectoriesOrDie(parent_dir); } - if (it->second.empty()) { + if (it.second.empty()) { // Create an empty file - HANDLE h = CreateFileW(it->first.c_str(), // name of the file + HANDLE h = CreateFileW(it.first.c_str(), // name of the file GENERIC_WRITE, // open for writing 0, // sharing mode, none in this case 0, // use default security descriptor CREATE_ALWAYS, // overwrite if exists FILE_ATTRIBUTE_NORMAL, 0); - if (h) { + if (h != INVALID_HANDLE_VALUE) { CloseHandle(h); } else { die(L"CreateFileW failed (%s): %hs", - it->first.c_str(), GetLastErrorString().c_str()); + it.first.c_str(), GetLastErrorString().c_str()); } } else { DWORD create_dir = 0; - if (blaze_util::IsDirectoryW(it->second.c_str())) { + if (blaze_util::IsDirectoryW(it.second.c_str())) { create_dir = SYMBOLIC_LINK_FLAG_DIRECTORY; } - if (!CreateSymbolicLinkW(it->first.c_str(), - it->second.c_str(), + if (!CreateSymbolicLinkW(it.first.c_str(), + it.second.c_str(), privilege_flag | create_dir)) { die(L"CreateSymbolicLinkW failed (%s -> %s): %hs", - it->first.c_str(), it->second.c_str(), + it.first.c_str(), it.second.c_str(), GetLastErrorString().c_str()); } } @@ -348,7 +347,7 @@ class RunfilesCreator { wstring new_manifest_file = runfiles_output_base_ + L"\\MANIFEST"; if (!CopyFileW(manifest_path_.c_str(), new_manifest_file.c_str(), - /*bFailIfExists=*/ false + /*bFailIfExists=*/ FALSE )) { die(L"CopyFileW failed (%s -> %s): %hs", manifest_path_.c_str(), new_manifest_file.c_str(), @@ -381,7 +380,7 @@ int wmain(int argc, wchar_t** argv) { if (argc != 2) { fprintf(stderr, "usage: [--allow_relative] [--use_metadata] " - "INPUT RUNFILES\n"); + "manifest_file runfiles_base_dir\n"); return 1; } diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index 12a817361a8086..a0702d9a52282c 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -54,22 +54,17 @@ esac if "$is_windows"; then export MSYS_NO_PATHCONV=1 export MSYS2_ARG_CONV_EXCL="*" + export EXT=".exe" + export EXTRA_BUILD_FLAGS="--experimental_enable_runfiles --build_python_zip=0" +else + export EXT="" + export EXTRA_BUILD_FLAGS="" fi #### SETUP ############################################################# set -e -function set_up() { - if "$is_windows"; then - export EXT=".exe" - export EXTRA_BUILD_FLAGS="--experimental_enable_runfiles --build_python_zip=0" - else - export EXT="" - export EXTRA_BUILD_FLAGS="" - fi -} - function create_pkg() { local -r pkg=$1 mkdir -p $pkg @@ -199,14 +194,15 @@ EOF fi for i in $(find ${WORKSPACE_NAME} \! -type d); do - if readlink "$i" > /dev/null; then + target="$(readlink "$i" || true)" + if [[ -z "$target" ]]; then + echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 + else if "$is_windows"; then - echo "$i $(cygpath -m $(readlink "$i"))" >> ${TEST_TMPDIR}/MANIFEST2 + echo "$i $(cygpath -m $target)" >> ${TEST_TMPDIR}/MANIFEST2 else - echo "$i $(readlink "$i")" >> ${TEST_TMPDIR}/MANIFEST2 + echo "$i $target" >> ${TEST_TMPDIR}/MANIFEST2 fi - else - echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 fi done sort MANIFEST > ${TEST_TMPDIR}/MANIFEST_sorted @@ -274,14 +270,15 @@ EOF rm -f ${TEST_TMPDIR}/MANIFEST rm -f ${TEST_TMPDIR}/MANIFEST2 for i in $(find ${WORKSPACE_NAME} \! -type d); do - if readlink "$i" > /dev/null; then + target="$(readlink "$i" || true)" + if [[ -z "$target" ]]; then + echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 + else if "$is_windows"; then - echo "$i $(cygpath -m $(readlink "$i"))" >> ${TEST_TMPDIR}/MANIFEST2 + echo "$i $(cygpath -m $target)" >> ${TEST_TMPDIR}/MANIFEST2 else - echo "$i $(readlink "$i")" >> ${TEST_TMPDIR}/MANIFEST2 + echo "$i $target" >> ${TEST_TMPDIR}/MANIFEST2 fi - else - echo "$i " >> ${TEST_TMPDIR}/MANIFEST2 fi done sort MANIFEST > ${TEST_TMPDIR}/MANIFEST_sorted From 2e9b00e7ec190b2ea94ee0317393e2c7b6edbbb5 Mon Sep 17 00:00:00 2001 From: Yun Peng Date: Thu, 30 Aug 2018 13:06:25 +0200 Subject: [PATCH 3/3] Add more comments Change-Id: I2e1fb56f7686872f5e7d4724c93c1ad2e6e02311 --- src/main/tools/build-runfiles-windows.cc | 23 +++++++++++++++------ src/test/shell/integration/runfiles_test.sh | 4 ++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index 9f82e536b937a7..db01284e0c2635 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/src/main/tools/build-runfiles-windows.cc @@ -121,7 +121,7 @@ class RunfilesCreator { } } - void ReadManifest(bool allow_relative, bool use_metadata) { + void ReadManifest(bool allow_relative, bool ignore_metadata) { ifstream manifest_file( AsAbsoluteWindowsPath(manifest_path_.c_str()).c_str()); @@ -135,7 +135,7 @@ class RunfilesCreator { lineno++; // Skip metadata lines. They are used solely for // dependency checking. - if (use_metadata && lineno % 2 == 0) { + if (ignore_metadata && lineno % 2 == 0) { continue; } @@ -154,6 +154,11 @@ class RunfilesCreator { Trim(link); Trim(target); + + // We sometimes need to create empty files under the runfiles tree. + // For example, for python binary, __init__.py is needed under every directory. + // Storing an entry with an empty target indicates we need to create such a + // file when creating the runfiles tree. if (!allow_relative && !target.empty() && !blaze_util::IsAbsolute(target)) { die(L"Target cannot be relative path: %hs", line.c_str()); @@ -270,6 +275,7 @@ class RunfilesCreator { if (expected_target == manifest_file_map.end() || expected_target->second.empty() + // Both paths are normalized paths in lower case, we can compare them directly. || target != AsAbsoluteWindowsPath(expected_target->second.c_str()) || blaze_util::IsDirectoryW(target) != is_dir) { if (is_dir) { @@ -286,6 +292,9 @@ class RunfilesCreator { // If the directory is empty, then we remove the directory. // Otherwise RemoveDirectory will fail with ERROR_DIR_NOT_EMPTY, // which we can just ignore. + // Because if the directory is not empty, it means it contains some symlinks + // already pointing to the correct targets (we just called ScanTreeAndPrune). + // Then this directory shouldn't be removed in the first place. if (!RemoveDirectoryW(subpath.c_str()) && GetLastError() != ERROR_DIR_NOT_EMPTY) { die(L"RemoveDirectoryW failed (%s): %hs", @@ -364,14 +373,16 @@ class RunfilesCreator { int wmain(int argc, wchar_t** argv) { argc--; argv++; bool allow_relative = false; - bool use_metadata = false; + bool ignore_metadata = false; while (argc >= 1) { if (wcscmp(argv[0], L"--allow_relative") == 0) { allow_relative = true; argc--; argv++; } else if (wcscmp(argv[0], L"--use_metadata") == 0) { - use_metadata = true; + // If --use_metadata is passed, it means manifest file contains metadata lines, + // which we should ignore when reading manifest file. + ignore_metadata = true; argc--; argv++; } else { break; @@ -380,7 +391,7 @@ int wmain(int argc, wchar_t** argv) { if (argc != 2) { fprintf(stderr, "usage: [--allow_relative] [--use_metadata] " - "manifest_file runfiles_base_dir\n"); + " \n"); return 1; } @@ -392,7 +403,7 @@ int wmain(int argc, wchar_t** argv) { RunfilesCreator runfiles_creator(manifest_absolute_path, output_base_absolute_path); - runfiles_creator.ReadManifest(allow_relative, use_metadata); + runfiles_creator.ReadManifest(allow_relative, ignore_metadata); runfiles_creator.CreateRunfiles(); return 0; diff --git a/src/test/shell/integration/runfiles_test.sh b/src/test/shell/integration/runfiles_test.sh index a0702d9a52282c..fd85be9849fd82 100755 --- a/src/test/shell/integration/runfiles_test.sh +++ b/src/test/shell/integration/runfiles_test.sh @@ -179,6 +179,8 @@ EOF # that accounts for everything cd ../.. + # For shell binary, we build both `bin` and `bin.exe`, but on Linux we only build `bin` + # That's why we have one more symlink on Windows. if "$is_windows"; then assert_equals 10 $(find ${WORKSPACE_NAME} -type l | wc -l) assert_equals 4 $(find ${WORKSPACE_NAME} -type f | wc -l) @@ -253,6 +255,8 @@ EOF # that accounts for everything cd ../.. + # For shell binary, we build both `bin` and `bin.exe`, but on Linux we only build `bin` + # That's why we have one more symlink on Windows. if "$is_windows"; then assert_equals 4 $(find ${WORKSPACE_NAME} -type l | wc -l) assert_equals 0 $(find ${WORKSPACE_NAME} -type f | wc -l)