-
Notifications
You must be signed in to change notification settings - Fork 425
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.
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
Prevent non-atomic writes to repodata JSON files #3834
Conversation
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @sscherfke. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
The link to the CLA stuff in the README seems to be broken: https://www.clahub.com/agreements/conda/conda-build |
Hm, |
We require contributors to sign our Contributor License Agreement, and we don't have one on file for @sscherfke. In order for us to review and merge your code, please e-sign the PDF at https://conda.io/en/latest/contributing.html#conda-contributor-license-agreement. We then need to manually verify your signature. We will ping the bot to refresh the PR status when we have confirmed your signature. |
Using Also signed the CLA. |
CHANGELOG.txt
Outdated
@@ -79,7 +85,7 @@ Enhancements: | |||
------------- | |||
|
|||
* add --use-channeldata argument to conda render/build. | |||
* Extract the part in the skeletons pypi responsible to get the package metadata to a free function. | |||
* Extract the part in the skeletons pypi responsible to get the package metadata to a free function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please revert this file?
You can add the description and you changelog here
https://github.com/conda/conda-build/blob/master/news/
and you can also use our template
https://github.com/conda/conda-build/blob/master/news/TEMPLATE
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add a new rst
file on that folder using our template and it should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it look okay now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file is still here =p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please revert it? After that, I think that should be good to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought I had reverted it. Tried it again. :)
temp_path = join(gettempdir(), str(uuid4())) | ||
# Create the temp file next "path" so that we can use an atomic move, see | ||
# https://github.com/conda/conda-build/issues/3833 | ||
temp_path = '%s.%s' % (path, uuid4()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that possible to reproduce the error using a test?
It would be interesting to have a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m afraid that’s no easily possible.
The error was that path
and the temp_path
were on two different file systems and temp_file
was big enough that there could happen a read on path
while temp_path
was being copied into it.
Now, both files are always in the same folder and thus on the same filesystem.
Can this be merged? :) |
Sorry for the delay @sscherfke , because of the end of the year, holidays and all that stuff we might be a bit slower than usual 😄 |
The changelog should now be clean. |
Thanks for your contribution @sscherfke ! 🎉 |
Thanks for your kind feedback 🙂 |
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! |
Fixes: #3833