fixing bug that could cause to silently overwrite an existing easyconfig when using --try-*#2635
Conversation
|
Tagging @ocaisa |
| print_warning("Overwriting existing file at %s with tweaked easyconfig file (due to --force)", tweaked_spec) | ||
| else: | ||
| raise EasyBuildError("A file already exists at %s where tweaked easyconfig file would be written", | ||
| tweaked_spec) |
There was a problem hiding this comment.
@mboisson Shouldn't we do this in dump itself instead? We may be using dump is other places in the future...
There was a problem hiding this comment.
We could. I went to the simplest and least intrusive. Note that there are two different code-paths that can write files, in tweak.py. The first one uses write_file, the other one uses dump. The test was already around the write_file call, but not around the dump part. I am happy with whatever fix you or @ocaisa think is best.
There was a problem hiding this comment.
In this case I really think doing it in dump itself makes more sense...
I'm not sure why there are two code paths where one uses write_file and another uses dump, maybe @ocaisa knows?
There was a problem hiding this comment.
There are two paths because one uses regex substitution and the other modifies the EasyConfig object and dumps the modified object.
Ultimately the dump method does use write_file so perhaps the question is whether we want to pass down options, the choices currently are:
def write_file(path, txt, append=False, forced=False, backup=False):
"""
Write given contents to file at given path;
overwrites current file contents without backup by default!
:param path: location of file
:param txt: contents to write to file
:param append: append to existing file rather than overwrite
:param forced: force actually writing file in (extended) dry run mode
:param backup: back up existing file before overwriting or modifying it
"""
There was a problem hiding this comment.
Perhaps doing a backup is sufficient? That way nothing ever gets removed and no need for the force option
There was a problem hiding this comment.
I think write_file should at the very least issue a warning if not fail if it is going to be over-writing an existing file.
There was a problem hiding this comment.
If a backup is created, it will print a warning message for that.
But I agree with @mboisson here, it shouldn't overwrite existing files without --force (and it should still create a backup if --force is used).
There was a problem hiding this comment.
@mboisson W.r.t. making write_file fail hard rather than overwriting an existing file: we can't do that without breaking backward compatibility, changing that would likely break a lot of easyblocks. Also, always having to use force to make write_file actually overwrite existing file would be a bit much, that's not standard semantics in Linux either...
There was a problem hiding this comment.
So, how do we proceed? Are you saying that the original check that @mboisson had is appropriate then and maybe couple that with adding the backup kw in the dump?
There was a problem hiding this comment.
I took a look at it, the best way is to enhance write_file a bit to optionally not blindly overwrite existing files, and then use write_file correctly in both places.
Done in ComputeCanada#8
|
@mboisson Also, we should get this covered in the tests, i.e. have a test that verifies that |
…s --force is used, and to be verbose about creating backups
different approach to avoid blind overwriting of existing files with --try-*
fix tweak_one test + remove duplicate check for forced overwriting of existing file
|
@ocaisa Thoughts on current approach? |
| tweaked_ec) | ||
|
|
||
| write_file(tweaked_ec, ectxt) | ||
| write_file(tweaked_ec, ectxt, backup=True, overwrite=False, verbose=True) |
There was a problem hiding this comment.
What's the point of this? If you're not going to overwrite then why backup? Shouldn't backup imply overwrite?
There was a problem hiding this comment.
Ah ok, I understand it now but the kw name is not very helpful, how about unforced_overwrite?
There was a problem hiding this comment.
overwrite=False means "don't overwrite unless --force is used"
Without also adding backup=True, you may overwrite when using --force without backing up the file.
Whether you want a backup or not depends on in what context you're calling write_file.
There was a problem hiding this comment.
Ok, but the kw name needs tweaking, it's not unreasonable to expect overwrite=False not to overwrite.
There was a problem hiding this comment.
I had overwrite_without_force before, which I found too long, but would that be better?
There was a problem hiding this comment.
how about always_overwrite ?
There was a problem hiding this comment.
overwrite_without_force is long but since the option is so connected to --force I think it is worth the keystrokes, it will save people having to read the docstring
There was a problem hiding this comment.
@mboisson always_overwrite makes sense to me. Can you make that change?
There was a problem hiding this comment.
I'm kind of lost in all of the changes you PR'd to us now :/
| tweaked_spec = os.path.join(targetdir or tempfile.gettempdir(), ec_filename) | ||
| parsed_ec['ec'].dump(tweaked_spec) | ||
|
|
||
| parsed_ec['ec'].dump(tweaked_spec, overwrite=False, backup=True) |
fix write_file tests after 'overwrite' argument was renamed to 'always_overwrite'
boegel
changed the title
3 participants
No description provided.