From b8562feedce09dd32079754ac6705d43e522da53 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Thu, 9 May 2024 15:54:58 -0400 Subject: [PATCH 1/5] Automatically figure out where to write GIT_COMMIT In most cases, we will want to put GIT_COMMIT in a directory with the same name as the project (with hyphens replaced by underscores.) Put GIT_COMMIT there by default. Also rename _edit_git_commit() to _write_git_commit(), and don't create a backup file with the old GIT_COMMIT. --- README.md | 16 +++++----- rapids_build_backend/impls.py | 42 +++++++++++-------------- tests/test_impls.py | 59 ++++++++++++++++++++--------------- tests/test_packages.py | 4 +-- 4 files changed, 62 insertions(+), 59 deletions(-) diff --git a/README.md b/README.md index 22a1277..09f3fff 100644 --- a/README.md +++ b/README.md @@ -15,14 +15,14 @@ In cases where more dynamic customization is sensible, suitable environment vari Any option without a default is required. -| Option | Definition | Type | Default | Supports dynamic modification | -|-----------------------|---------------------------------------------------------------------|----------------|---------------------|-------------------------------| -| `build-backend` | The wrapped build backend (e.g. `setuptools.build_meta`) | string | | N | -| `commit-file` | The file in which to write the git commit hash | string | "" (No file) | N | -| `dependencies-file` | The path to the `dependencies.yaml` file to use | string | "dependencies.yaml" | Y | -| `matrix-entry` | A `;`-separated list of `=`-delimited key/value pairs | string | "" | Y | -| `require-cuda` | If false, builds will succeed even if nvcc is not available | bool | true | Y | -| `requires` | List of build requirements (in addition to `build-system.requires`) | list[str] | [] | N | +| Option | Definition | Type | Default | Supports dynamic modification | +|-----------------------|---------------------------------------------------------------------|----------------|-----------------------------|-------------------------------| +| `build-backend` | The wrapped build backend (e.g. `setuptools.build_meta`) | string | | N | +| `commit-file` | The file in which to write the git commit hash | string | "/GIT_COMMIT" | N | +| `dependencies-file` | The path to the `dependencies.yaml` file to use | string | "dependencies.yaml" | Y | +| `matrix-entry` | A `;`-separated list of `=`-delimited key/value pairs | string | "" | Y | +| `require-cuda` | If false, builds will succeed even if nvcc is not available | bool | true | Y | +| `requires` | List of build requirements (in addition to `build-system.requires`) | list[str] | [] | N | ## Outstanding questions diff --git a/rapids_build_backend/impls.py b/rapids_build_backend/impls.py index 3dec619..88c76f6 100644 --- a/rapids_build_backend/impls.py +++ b/rapids_build_backend/impls.py @@ -118,7 +118,7 @@ def _get_git_commit() -> Union[str, None]: @contextmanager -def _edit_git_commit(config): +def _write_git_commit(config, project_name): """ Temporarily modify the git commit of the package being built. @@ -126,29 +126,17 @@ def _edit_git_commit(config): at build time. """ commit_file = config.commit_file + if commit_file == "": + commit_file = os.path.join(project_name.replace("-", "_"), "GIT_COMMIT") commit = _get_git_commit() - if commit_file != "" and commit is not None: - bkp_commit_file = os.path.join( - os.path.dirname(commit_file), - f".{os.path.basename(commit_file)}.rapids-build-backend.bak", - ) + if commit_file is not None and commit is not None: + with open(commit_file, "w") as f: + f.write(f"{commit}\n") try: - try: - shutil.move(commit_file, bkp_commit_file) - except FileNotFoundError: - bkp_commit_file = None - - with open(commit_file, "w") as f: - f.write(f"{commit}\n") - yield finally: - # Restore by moving rather than writing to avoid any formatting changes. - if bkp_commit_file: - shutil.move(bkp_commit_file, commit_file) - else: - os.unlink(commit_file) + os.unlink(commit_file) else: yield @@ -227,7 +215,9 @@ def _edit_pyproject(config): # (the actual build backend, which conditionally imports these functions). def get_requires_for_build_wheel(config_settings): config = Config(config_settings=config_settings) - with _edit_pyproject(config), _edit_git_commit(config): + pyproject = utils._get_pyproject() + project_name = pyproject["project"]["name"] + with _edit_pyproject(config), _write_git_commit(config, project_name): # Reload the config for a possibly updated tool.rapids-build-backend.requires reloaded_config = Config(config_settings=config_settings) requires = list(reloaded_config.requires) @@ -243,7 +233,9 @@ def get_requires_for_build_wheel(config_settings): def get_requires_for_build_sdist(config_settings): config = Config(config_settings=config_settings) - with _edit_pyproject(config), _edit_git_commit(config): + pyproject = utils._get_pyproject() + project_name = pyproject["project"]["name"] + with _edit_pyproject(config), _write_git_commit(config, project_name): # Reload the config for a possibly updated tool.rapids-build-backend.requires reloaded_config = Config(config_settings=config_settings) requires = list(reloaded_config.requires) @@ -275,7 +267,9 @@ def get_requires_for_build_editable(config_settings): def build_wheel(wheel_directory, config_settings=None, metadata_directory=None): config = Config(config_settings=config_settings) - with _edit_pyproject(config), _edit_git_commit(config): + pyproject = utils._get_pyproject() + project_name = pyproject["project"]["name"] + with _edit_pyproject(config), _write_git_commit(config, project_name): return _get_backend(config.build_backend).build_wheel( wheel_directory, config_settings, metadata_directory ) @@ -283,7 +277,9 @@ def build_wheel(wheel_directory, config_settings=None, metadata_directory=None): def build_sdist(sdist_directory, config_settings=None): config = Config(config_settings=config_settings) - with _edit_pyproject(config), _edit_git_commit(config): + pyproject = utils._get_pyproject() + project_name = pyproject["project"]["name"] + with _edit_pyproject(config), _write_git_commit(config, project_name): return _get_backend(config.build_backend).build_sdist( sdist_directory, config_settings ) diff --git a/tests/test_impls.py b/tests/test_impls.py index c537b02..5d547eb 100644 --- a/tests/test_impls.py +++ b/tests/test_impls.py @@ -9,9 +9,9 @@ import pytest from rapids_build_backend.impls import ( - _edit_git_commit, _edit_pyproject, _get_cuda_suffix, + _write_git_commit, ) @@ -26,39 +26,46 @@ def set_cwd(cwd): @pytest.mark.parametrize( - "initial_contents", + ("project_name", "directory", "commit_file_config", "expected_commit_file"), [ - "def456\n", - "", - None, + ("test-project", "test_project", "", "test_project/GIT_COMMIT"), + ( + "test-project", + "_test_project", + "_test_project/GIT_COMMIT", + "_test_project/GIT_COMMIT", + ), + ("test-project", None, None, None), ], ) @patch("rapids_build_backend.impls._get_git_commit", Mock(return_value="abc123")) -def test_edit_git_commit(initial_contents): - def check_initial_contents(filename): - if initial_contents is not None: - with open(filename) as f: - assert f.read() == initial_contents - else: - assert not os.path.exists(filename) - - with tempfile.TemporaryDirectory() as d: - commit_file = os.path.join(d, "commit-file") - bkp_commit_file = os.path.join(d, ".commit-file.rapids-build-backend.bak") - if initial_contents is not None: - with open(commit_file, "w") as f: - f.write(initial_contents) +def test_write_git_commit( + tmp_path, project_name, directory, commit_file_config, expected_commit_file +): + with set_cwd(tmp_path): + if directory: + os.mkdir(directory) config = Mock( - commit_file=commit_file, + commit_file=commit_file_config, ) - with _edit_git_commit(config): - with open(commit_file) as f: - assert f.read() == "abc123\n" - check_initial_contents(bkp_commit_file) + with _write_git_commit(config, project_name): + if expected_commit_file: + with open(expected_commit_file) as f: + assert f.read() == "abc123\n" + else: + assert list(os.walk(".")) == [ + (".", [], []), + ] - assert not os.path.exists(bkp_commit_file) - check_initial_contents(commit_file) + if directory: + assert list(os.walk(directory)) == [ + (directory, [], []), + ] + os.rmdir(directory) + assert list(os.walk(".")) == [ + (".", [], []), + ] @pytest.mark.parametrize( diff --git a/tests/test_packages.py b/tests/test_packages.py index 590de83..659582f 100644 --- a/tests/test_packages.py +++ b/tests/test_packages.py @@ -67,7 +67,7 @@ def test_simple_setuptools(tmp_path, env, nvcc_version): } package_dir = tmp_path / "pkg" - os.makedirs(package_dir) + os.makedirs(package_dir / "simple_setuptools") generate_from_template(package_dir, "dependencies.yaml", template_args) generate_from_template(package_dir, "pyproject.toml", template_args) @@ -99,7 +99,7 @@ def test_simple_scikit_build_core(tmp_path, env, nvcc_version): } package_dir = tmp_path / "pkg" - os.makedirs(package_dir) + os.makedirs(package_dir / "simple_scikit_build_core") generate_from_template(package_dir, "dependencies.yaml", template_args) generate_from_template(package_dir, "pyproject.toml", template_args) From 8476cad174b9e7b4e790ea108767db1cd2194c81 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Thu, 9 May 2024 16:16:58 -0400 Subject: [PATCH 2/5] Add type annotation --- rapids_build_backend/impls.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rapids_build_backend/impls.py b/rapids_build_backend/impls.py index 88c76f6..fb47e8b 100644 --- a/rapids_build_backend/impls.py +++ b/rapids_build_backend/impls.py @@ -118,7 +118,7 @@ def _get_git_commit() -> Union[str, None]: @contextmanager -def _write_git_commit(config, project_name): +def _write_git_commit(config, project_name: str): """ Temporarily modify the git commit of the package being built. From 6d7ad159a8dccffd6aa11f8cff40ff1947f11f0a Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Thu, 9 May 2024 16:40:42 -0400 Subject: [PATCH 3/5] Don't allow commit-file to be None --- rapids_build_backend/impls.py | 2 +- tests/test_impls.py | 16 +++------------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/rapids_build_backend/impls.py b/rapids_build_backend/impls.py index fb47e8b..c511c1d 100644 --- a/rapids_build_backend/impls.py +++ b/rapids_build_backend/impls.py @@ -130,7 +130,7 @@ def _write_git_commit(config, project_name: str): commit_file = os.path.join(project_name.replace("-", "_"), "GIT_COMMIT") commit = _get_git_commit() - if commit_file is not None and commit is not None: + if commit is not None: with open(commit_file, "w") as f: f.write(f"{commit}\n") try: diff --git a/tests/test_impls.py b/tests/test_impls.py index 5d547eb..87079ce 100644 --- a/tests/test_impls.py +++ b/tests/test_impls.py @@ -35,7 +35,6 @@ def set_cwd(cwd): "_test_project/GIT_COMMIT", "_test_project/GIT_COMMIT", ), - ("test-project", None, None, None), ], ) @patch("rapids_build_backend.impls._get_git_commit", Mock(return_value="abc123")) @@ -50,19 +49,10 @@ def test_write_git_commit( commit_file=commit_file_config, ) with _write_git_commit(config, project_name): - if expected_commit_file: - with open(expected_commit_file) as f: - assert f.read() == "abc123\n" - else: - assert list(os.walk(".")) == [ - (".", [], []), - ] + with open(expected_commit_file) as f: + assert f.read() == "abc123\n" - if directory: - assert list(os.walk(directory)) == [ - (directory, [], []), - ] - os.rmdir(directory) + os.rmdir(directory) assert list(os.walk(".")) == [ (".", [], []), ] From 7db44cab3a369aacc541480a9d2f63bc41253463 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 10 May 2024 10:29:41 -0400 Subject: [PATCH 4/5] Document _write_git_commit() --- rapids_build_backend/impls.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rapids_build_backend/impls.py b/rapids_build_backend/impls.py index c511c1d..a280529 100644 --- a/rapids_build_backend/impls.py +++ b/rapids_build_backend/impls.py @@ -120,7 +120,8 @@ def _get_git_commit() -> Union[str, None]: @contextmanager def _write_git_commit(config, project_name: str): """ - Temporarily modify the git commit of the package being built. + Temporarily write the git commit file for the package being built. If the + `commit-file` config option is not specified, write to `/GIT_COMMIT`. This is useful for projects that want to embed the current git commit in the package at build time. From 0c3a026093908c58b22ea3ec22453fa9c858f2b0 Mon Sep 17 00:00:00 2001 From: Kyle Edwards Date: Fri, 10 May 2024 17:50:25 -0400 Subject: [PATCH 5/5] Make commit-files zero-or-more --- README.md | 16 ++++++------ rapids_build_backend/config.py | 24 +++++++++-------- rapids_build_backend/impls.py | 30 +++++++++++---------- tests/test_config.py | 5 ++-- tests/test_impls.py | 48 ++++++++++++++++++++++------------ 5 files changed, 71 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 09f3fff..cd3494a 100644 --- a/README.md +++ b/README.md @@ -15,14 +15,14 @@ In cases where more dynamic customization is sensible, suitable environment vari Any option without a default is required. -| Option | Definition | Type | Default | Supports dynamic modification | -|-----------------------|---------------------------------------------------------------------|----------------|-----------------------------|-------------------------------| -| `build-backend` | The wrapped build backend (e.g. `setuptools.build_meta`) | string | | N | -| `commit-file` | The file in which to write the git commit hash | string | "/GIT_COMMIT" | N | -| `dependencies-file` | The path to the `dependencies.yaml` file to use | string | "dependencies.yaml" | Y | -| `matrix-entry` | A `;`-separated list of `=`-delimited key/value pairs | string | "" | Y | -| `require-cuda` | If false, builds will succeed even if nvcc is not available | bool | true | Y | -| `requires` | List of build requirements (in addition to `build-system.requires`) | list[str] | [] | N | +| Option | Definition | Type | Default | Supports dynamic modification | +|-----------------------|---------------------------------------------------------------------|----------------|-------------------------------|-------------------------------| +| `build-backend` | The wrapped build backend (e.g. `setuptools.build_meta`) | string | | N | +| `commit-files` | List of files in which to write the git commit hash | list[str] | ["/GIT_COMMIT"] | N | +| `dependencies-file` | The path to the `dependencies.yaml` file to use | string | "dependencies.yaml" | Y | +| `matrix-entry` | A `;`-separated list of `=`-delimited key/value pairs | string | "" | Y | +| `require-cuda` | If false, builds will succeed even if nvcc is not available | bool | true | Y | +| `requires` | List of build requirements (in addition to `build-system.requires`) | list[str] | [] | N | ## Outstanding questions diff --git a/rapids_build_backend/config.py b/rapids_build_backend/config.py index 4775dfb..ef7d022 100644 --- a/rapids_build_backend/config.py +++ b/rapids_build_backend/config.py @@ -16,23 +16,23 @@ config_val_callable = Callable[[], Union[config_val_type, mutable_config_val_type]] config_options_type = dict[ - str, tuple[Union[config_val_type, config_val_callable], bool] + str, tuple[Union[config_val_type, config_val_callable], bool, bool] ] class Config: """Manage the build configuration for the project.""" - # Mapping from config option to default value (None indicates that option is - # required) and whether it may be overridden by an environment variable or a config + # Mapping from config option to default value, whether it's required, and + # whether it may be overridden by an environment variable or a config # setting. config_options: "config_options_type" = { - "build-backend": (None, False), - "commit-file": ("", False), - "dependencies-file": ("dependencies.yaml", True), - "matrix-entry": ("", True), - "require-cuda": (True, True), - "requires": (lambda: [], False), + "build-backend": (None, True, False), + "commit-files": (None, False, False), + "dependencies-file": ("dependencies.yaml", False, True), + "matrix-entry": ("", False, True), + "require-cuda": (True, False, True), + "requires": (lambda: [], False, False), } def __init__(self, dirname=".", config_settings=None): @@ -46,7 +46,9 @@ def __init__(self, dirname=".", config_settings=None): def __getattr__(self, name): config_name = name.replace("_", "-") if config_name in Config.config_options: - default_value, allows_override = Config.config_options[config_name] + default_value, required, allows_override = Config.config_options[ + config_name + ] if callable(default_value): default_value = default_value() @@ -74,7 +76,7 @@ def __getattr__(self, name): try: return self.config[config_name] except KeyError: - if default_value is not None: + if not required: return default_value raise AttributeError(f"Config is missing required attribute '{name}'") diff --git a/rapids_build_backend/impls.py b/rapids_build_backend/impls.py index a280529..e867da8 100644 --- a/rapids_build_backend/impls.py +++ b/rapids_build_backend/impls.py @@ -118,26 +118,28 @@ def _get_git_commit() -> Union[str, None]: @contextmanager -def _write_git_commit(config, project_name: str): +def _write_git_commits(config, project_name: str): """ - Temporarily write the git commit file for the package being built. If the - `commit-file` config option is not specified, write to `/GIT_COMMIT`. + Temporarily write the git commit files for the package being built. If the + `commit-files` config option is not specified, write to `/GIT_COMMIT`. This is useful for projects that want to embed the current git commit in the package at build time. """ - commit_file = config.commit_file - if commit_file == "": - commit_file = os.path.join(project_name.replace("-", "_"), "GIT_COMMIT") - commit = _get_git_commit() + commit_files = config.commit_files + if commit_files is None: + commit_files = [os.path.join(project_name.replace("-", "_"), "GIT_COMMIT")] + commit = _get_git_commit() if commit_files else None if commit is not None: - with open(commit_file, "w") as f: - f.write(f"{commit}\n") + for commit_file in commit_files: + with open(commit_file, "w") as f: + f.write(f"{commit}\n") try: yield finally: - os.unlink(commit_file) + for commit_file in commit_files: + os.unlink(commit_file) else: yield @@ -218,7 +220,7 @@ def get_requires_for_build_wheel(config_settings): config = Config(config_settings=config_settings) pyproject = utils._get_pyproject() project_name = pyproject["project"]["name"] - with _edit_pyproject(config), _write_git_commit(config, project_name): + with _edit_pyproject(config), _write_git_commits(config, project_name): # Reload the config for a possibly updated tool.rapids-build-backend.requires reloaded_config = Config(config_settings=config_settings) requires = list(reloaded_config.requires) @@ -236,7 +238,7 @@ def get_requires_for_build_sdist(config_settings): config = Config(config_settings=config_settings) pyproject = utils._get_pyproject() project_name = pyproject["project"]["name"] - with _edit_pyproject(config), _write_git_commit(config, project_name): + with _edit_pyproject(config), _write_git_commits(config, project_name): # Reload the config for a possibly updated tool.rapids-build-backend.requires reloaded_config = Config(config_settings=config_settings) requires = list(reloaded_config.requires) @@ -270,7 +272,7 @@ def build_wheel(wheel_directory, config_settings=None, metadata_directory=None): config = Config(config_settings=config_settings) pyproject = utils._get_pyproject() project_name = pyproject["project"]["name"] - with _edit_pyproject(config), _write_git_commit(config, project_name): + with _edit_pyproject(config), _write_git_commits(config, project_name): return _get_backend(config.build_backend).build_wheel( wheel_directory, config_settings, metadata_directory ) @@ -280,7 +282,7 @@ def build_sdist(sdist_directory, config_settings=None): config = Config(config_settings=config_settings) pyproject = utils._get_pyproject() project_name = pyproject["project"]["name"] - with _edit_pyproject(config), _write_git_commit(config, project_name): + with _edit_pyproject(config), _write_git_commits(config, project_name): return _get_backend(config.build_backend).build_sdist( sdist_directory, config_settings ) diff --git a/tests/test_config.py b/tests/test_config.py index ca415f4..73edc2f 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -25,8 +25,9 @@ def setup_config_project(tmp_path, flag, config_value): @pytest.mark.parametrize( "flag, config_value, expected", [ - ("commit-file", '"pkg/_version.py"', "pkg/_version.py"), - ("commit-file", None, ""), + ("commit-files", '["pkg/_version.py"]', ["pkg/_version.py"]), + ("commit-files", "[]", []), + ("commit-files", None, None), ("require-cuda", "true", True), ("require-cuda", "false", False), ("require-cuda", None, True), diff --git a/tests/test_impls.py b/tests/test_impls.py index 87079ce..da7da72 100644 --- a/tests/test_impls.py +++ b/tests/test_impls.py @@ -11,7 +11,7 @@ from rapids_build_backend.impls import ( _edit_pyproject, _get_cuda_suffix, - _write_git_commit, + _write_git_commits, ) @@ -26,36 +26,50 @@ def set_cwd(cwd): @pytest.mark.parametrize( - ("project_name", "directory", "commit_file_config", "expected_commit_file"), + ("project_name", "directories", "commit_files_config", "expected_commit_files"), [ - ("test-project", "test_project", "", "test_project/GIT_COMMIT"), + ("test-project", ["test_project"], None, ["test_project/GIT_COMMIT"]), ( "test-project", - "_test_project", - "_test_project/GIT_COMMIT", - "_test_project/GIT_COMMIT", + ["_test_project"], + ["_test_project/GIT_COMMIT"], + ["_test_project/GIT_COMMIT"], + ), + ( + "test-project", + ["_test_project_1", "_test_project_2"], + ["_test_project_1/GIT_COMMIT", "_test_project_2/GIT_COMMIT"], + ["_test_project_1/GIT_COMMIT", "_test_project_2/GIT_COMMIT"], + ), + ( + "test-project", + [], + [], + [], ), ], ) @patch("rapids_build_backend.impls._get_git_commit", Mock(return_value="abc123")) -def test_write_git_commit( - tmp_path, project_name, directory, commit_file_config, expected_commit_file +def test_write_git_commits( + tmp_path, project_name, directories, commit_files_config, expected_commit_files ): with set_cwd(tmp_path): - if directory: + for directory in directories: os.mkdir(directory) config = Mock( - commit_file=commit_file_config, + commit_files=commit_files_config, ) - with _write_git_commit(config, project_name): - with open(expected_commit_file) as f: - assert f.read() == "abc123\n" + with _write_git_commits(config, project_name): + for expected_commit_file in expected_commit_files: + with open(expected_commit_file) as f: + assert f.read() == "abc123\n" + if not directories: + assert list(os.walk(".")) == [(".", [], [])] - os.rmdir(directory) - assert list(os.walk(".")) == [ - (".", [], []), - ] + for directory in directories: + os.rmdir(directory) + assert list(os.walk(".")) == [(".", [], [])] @pytest.mark.parametrize(