Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

verify that docker script reached its end #454

Merged
merged 3 commits into from
Feb 19, 2017
Merged

verify that docker script reached its end #454

merged 3 commits into from
Feb 19, 2017

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 8, 2017

protects against early termination appearing as success.

This is one small piece of #337, but might be reviewed and merged more easily because it doesn't change anything, just adds a check that the script finished executing.

This would at least turn the cases fixed by #337 into failures, rather than mysteriously missing packages.

@minrk
verify that docker script reached its end
4ef3b65 
protect against apparent early termination
@jakirkham
Copy link
Member

I think if we even just did || exit 1 after the docker run command we should get the same behavior more simply, no?

@minrk
Copy link
Member Author

minrk commented Feb 8, 2017 •
edited
Loading

I think that there already is an || exit $? on the docker run. One of the reasons this has slipped through is that whatever it was that mpiexec does, results in a clean (status=0) premature exit, rather than a nonzero status. Hence the canary file.

@minrk
Copy link
Member Author

minrk commented Feb 8, 2017

My rough understanding of what's happening:

  • mpiexec consumes (or closes) stdin when it is called
  • when bash resumes control, it reaches EOF without ever seeing commands issued after mpiexec, and exits as if everything is AOK

@jakirkham
Copy link
Member

jakirkham commented Feb 8, 2017 •
edited
Loading

Could it be that using $? instead of 1 is part of the problem. I'm not entirely sure why we bother with $? as I don't believe this is visible to the user.

Edit: Drop wrongly placed "not".

@minrk
Copy link
Member Author

minrk commented Feb 9, 2017

I don't think so. The || clause never executes because docker run exits with status 0 in these cases, indicating success.

@minrk
Copy link
Member Author

minrk commented Feb 9, 2017

this build was run after rendering with this PR, confirming that docker run exits cleanly and the || exit 1 is never reached. Only the canary check prevents the build from claiming to have succeeded.

@jakirkham
Copy link
Member

Thanks for the link. I agree with your conclusion.

jakirkham
jakirkham approved these changes Feb 9, 2017
View reviewed changes
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

LGTM

@jakirkham
Copy link
Member

@gqmelo, what do you think about this change?

gqmelo
gqmelo approved these changes Feb 11, 2017
View reviewed changes
Copy link
Contributor

@gqmelo gqmelo left a comment

Choose a reason for hiding this comment

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

LGTM

@jakirkham jakirkham added this to the 2.1.0 milestone Feb 12, 2017
@jakirkham jakirkham merged commit 4ee2b97 into conda-forge:master Feb 19, 2017
@jakirkham
Copy link
Member

Thanks @minrk. Thanks everyone who helped review.

@jakirkham jakirkham mentioned this pull request Feb 19, 2017
Merged
jakirkham
jakirkham reviewed Feb 19, 2017
View reviewed changes
conda_smithy/templates/run_docker_build.tmpl
@@ -50,5 +52,11 @@ conda build /recipe_root --quiet || exit 1
{%- endfor -%}
{%- endblock -%}

touch /feedstock_root/build_artefacts/conda-forge-build-done
EOF

Copy link
Member

Choose a reason for hiding this comment

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

Missed that we did not include another empty line below. As a result, this means the terminal newline would be dropped from this file. Fixing it in PR ( #459 ).

jakirkham
jakirkham reviewed Feb 19, 2017
View reviewed changes
conda_smithy/templates/run_docker_build.tmpl
# see https://github.com/conda-forge/conda-smithy/pull/337
# for a possible fix
set -x
test -f "$FEEDSTOCK_ROOT/build_artefacts/conda-forge-build-done" || exit 1
Copy link
Member

Choose a reason for hiding this comment

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

An empty line is added after here in PR ( #459 ) so the re-rendered file has a terminal newline.

@jakirkham jakirkham mentioned this pull request Apr 10, 2017
Merged
@jakirkham jakirkham mentioned this pull request Feb 19, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jakirkham jakirkham jakirkham approved these changes

@dalcinl dalcinl dalcinl approved these changes

@gqmelo gqmelo gqmelo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@minrk @jakirkham @dalcinl @gqmelo