Skip to content

Don't set build_id in test phase if --no-build-id is given#2100

Merged
msarahan merged 1 commit intoconda:2.1.xfrom
isuruf:patch-1
Jun 20, 2017
Merged

Don't set build_id in test phase if --no-build-id is given#2100
msarahan merged 1 commit intoconda:2.1.xfrom
isuruf:patch-1

Conversation

@isuruf
Copy link
Copy Markdown
Contributor

@isuruf isuruf commented Jun 16, 2017

Build work directory is deleted in test phase and it tries to
delete the work dir with id. As a consequence, the test phase
keeps the real work dir and then the next package build breaks
because it tries to apply a patch it was already applied.

cc @gqmelo

xref: conda-forge/conda-smithy#526 (comment)

Build work directory is deleted in test phase and it tries to
delete the work dir with id. As a consequence, the test phase
keeps the real work dir and then the next package build breaks
because it tries to apply a patch it was already applied.
@msarahan
Copy link
Copy Markdown
Contributor

Good fix. Is this worth adding a regression test for?

@isuruf
Copy link
Copy Markdown
Contributor Author

isuruf commented Jun 16, 2017

Sure. You'll have to guide me on it though

@gqmelo
Copy link
Copy Markdown
Contributor

gqmelo commented Jun 16, 2017

I created an issue before seeing this PR: #2101

@msarahan
Copy link
Copy Markdown
Contributor

I'd just start with @gqmelo's nice example:

mkdir -p /tmp/conda_build_id_bug/src
touch /tmp/conda_build_id_bug/src/somefile

echo "
package:
  name: foo
  version: 0.1
source:
  url: https://pypi.io/packages/source/e/exec-wrappers/exec-wrappers-1.0.1.tar.gz
" > /tmp/conda_build_id_bug/meta.yaml

echo "
test -f setup.py
rm setup.py
" > /tmp/conda_build_id_bug/build.sh

conda build --no-build-id /tmp/conda_build_id_bug
conda build --no-build-id /tmp/conda_build_id_bug

translated into a test, I'd move the echo stuff into files, and then instead of the conda build cli commands, I'd instead call api.build:

api.build(recipe_path, set_build_id=False)
api.build(recipe_path, set_build_id=False)

@msarahan
Copy link
Copy Markdown
Contributor

PS: test_api_build.py might be the right place for this test, but I'm not picky.

Comment thread conda_build/build.py
warn_on_use_of_SRC_DIR(metadata)

config.compute_build_id(metadata.name())
if config.set_build_id:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will make the test phase run without the build id, right?
Then before tests the work dir will be removed. But what happens if the package has no tests or they are not run? Wouldn't the next conda build command reuse the dirty work dir?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that without --dirty, the work dir would be cleaned up, but to be honest, I really never use --no-build-id. --no-build-id definitely breaks multiple builds happening at once, but it should still be clean for one build at a time.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'd like to add that in the example showed on #2101 if I replace the source url with a local path the work dir is overwritten and the example works

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my example has no tests at all, so it seems the work dir is really not cleaned up when starting a new build with --no-build-id.

Copy link
Copy Markdown
Contributor

@gqmelo gqmelo Jun 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I checked and the work dir with --no-build-id was not being deleted because the cleanup code was taking the work dir modified by the test phase even when there is no test. So this PR also fixes this case.

@msarahan
Copy link
Copy Markdown
Contributor

PPS: CLI arguments are not always exactly the same as the underlying configuration setting. In particular, I try to keep positive logic in the configuration. Look at https://github.com/conda/conda-build/blob/master/conda_build/config.py#L57 if you want to see what to pass the API call.

@msarahan
Copy link
Copy Markdown
Contributor

Thanks for the testing @gqmelo and @isuruf for the PR. Merging.

@msarahan msarahan merged commit b8532e2 into conda:2.1.x Jun 20, 2017
gqmelo added a commit to gqmelo/conda-build that referenced this pull request Jun 20, 2017
This test make sure the work dir is removed regardless of the --no-build-id option
gqmelo added a commit to gqmelo/conda-build that referenced this pull request Jun 20, 2017
This test make sure the work dir is removed regardless of the --no-build-id option
msarahan added a commit that referenced this pull request Jun 20, 2017
msarahan added a commit that referenced this pull request Jun 21, 2017
@msarahan msarahan mentioned this pull request Jun 24, 2017
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2022

Hi there, thank you for your contribution!

This pull request has been automatically locked because it has not had recent activity after being closed.

Please open a new issue or pull request if needed.

Thanks!

@github-actions github-actions Bot added the locked [bot] locked due to inactivity label May 1, 2022
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

locked [bot] locked due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants