Repository navigation
Conversation
|
The change LGTM, I just think of a strange case, |
19bf673 to
56a66da
Compare
|
Good catch, and it's worse than a failed build: run from Fixed in 56a66da: the build dir now must not be or contain the working directory, the template's directory, or |
sam build removes the build dir before building. aws#1403 refused a build dir equal to the current working directory, but other cases slipped through: - A build dir that contains the project: `sam build --build-dir ..`, or `sam build -t proj/template.yaml --build-dir proj` run from proj's parent, deletes the template and source (and, for `..`, everything beside the project). These are now refused: the build dir must not be or contain the working directory, the template's directory or the base dir. The comparison is case-insensitive on Windows. - .aws-sam itself, or the cache or deps dir: `--build-dir .aws-sam` deletes SAM CLI's own state on every build -- the build cache and dependencies (so --cached never hits) and the committed pipeline config written by `sam pipeline bootstrap` -- and writes build.toml into the project root. Some users may pass this today, so it now logs a warning that it will not be supported in a future release, and still builds. No warning is logged for a non-clean build, which removes nothing. .aws-sam/build (the default), .aws-sam/auto-dependency-layer (used by sam sync) and directories outside the project are unaffected.
56a66da to
140a76e
Compare
Which issue(s) does this change fix?
Fixes #9327
Why is this change necessary?
sam builddeletes--build-dirbefore it builds. The only guard (#1403) refuses the working directory itself. Sosam build --build-dir ..deletes the whole project and everything beside it, andsam build -t proj/template.yaml --build-dir projdeletes the template and its source.--build-dir .aws-sambuilds, but deletes SAM CLI's own build cache, dependencies and pipeline config on every run.How does it address the issue?
--build-dir.,.., any parent of the working directory.refused, parents deleted--base-dir, e.g.-t proj/template.yaml -b proj.aws-sam,.aws-sam/cache,.aws-sam/deps, or a dir containing--cache-dir.aws-sam/build(default),.aws-sam/auto-dependency-layer, any other dir_setup_build_dirinsamcli/commands/build/build_context.pycompares paths with a newis_same_or_ancestorhelper insamcli/lib/utils/path_utils.py. The helper usesos.path.commonpathandnormcase, so it ignores case on Windows. The.aws-samcase warns rather than fails, because some users pass it today. There is no warning whencleanis false, since nothing is removed then.DEFAULT_SAM_DIRis a new constant insamcli/commands/_utils/constants.py.What side effects does this change have?
--build-dirthat is or contains the working directory, the template's directory or the base dir now fails instead of deleting it.--build-dir .aws-samand similar now print a warning; the build itself is unchanged..aws-samentry overlaps with the deps-dir entry, which already sits inside.aws-sam. It is kept so the rule still holds if the deps dir ever moves.Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedNotes on the unchecked items:
Unit: the tests in
tests/unit/commands/buildcmd/test_build_context.pyandtests/unit/lib/utils/test_path_utils.pycover every row of the table on a real temporary filesystem. The helper also has Windows-path cases. Reverting either guard turns them red.Integration:
TestBuildCommand_BuildDirGuardintests/integration/buildcmd/test_build_cmd.pyruns a realsam buildfor four cases:..and../..are refused, and the project and a file next to it survive;-t proj/template.yaml --build-dir projis refused, and the template and source survive;.aws-samand.aws-sam/cachewarn and still build;Against released 1.166.2, the five guarded cases fail and the default case passes.
make pr:ruff,black,mypyand the build/sync/_utilsunit tests pass locally; the full suite is left to CI.--build-dirhelp text already says the directory is removed first.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.