Skip to content

fixing bug that could cause to silently overwrite an existing easyconfig when using --try-* #2635

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

Merged
boegel merged 12 commits into from
Nov 16, 2018
Merged

fixing bug that could cause to silently overwrite an existing easyconfig when using --try-* #2635

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ac07b75
fixing bug that could cause to silently overwrite an existing easyconfig
mboisson Oct 23, 2018
4a7b9d1
add support to write_file to prevent overwriting existing files unles…
boegel Nov 15, 2018
b299861
don't blindly overwrite existing easyconfig files with --try-*
boegel Nov 15, 2018
b69f2b0
Merge branch 'develop' into fix-bug-tweak
boegel Nov 15, 2018
54e7c20
Merge pull request #8 from boegel/fix-bug-tweak
mboisson Nov 15, 2018
1e8c2e0
fix tweak_one test + remove duplicate check for forced overwriting of…
boegel Nov 15, 2018
05718cc
Merge pull request #9 from boegel/fix-bug-tweak
mboisson Nov 15, 2018
c90bee8
renamed overwrite to always_overwrite
mboisson Nov 15, 2018
9fc02a2
fix write_file tests after 'overwrite' argument was renamed to 'alway…
boegel Nov 16, 2018
abb8aa8
fix typo (allways_overwrite -> always_overwrite)
boegel Nov 16, 2018
0c3e576
Merge pull request #10 from boegel/fix-bug-tweak
mboisson Nov 16, 2018
c7b3020
fixed docstring
mboisson Nov 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions easybuild/framework/easyconfig/tweak.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
:author: Toon Willems (Ghent University)
:author: Fotis Georgatos (Uni.Lu, NTUA)
:author: Alan O'Cais (Juelich Supercomputing Centre)
:author: Maxime Boissonneault (Universite Laval, Calcul Quebec, Compute Canada)
"""
import copy
import glob
Expand Down Expand Up @@ -836,6 +837,12 @@ def map_easyconfig_to_target_tc_hierarchy(ec_spec, toolchain_mapping, targetdir=
# Determine the name of the modified easyconfig and dump it to target_dir
ec_filename = '%s-%s.eb' % (parsed_ec['ec']['name'], det_full_ec_version(parsed_ec['ec']))
tweaked_spec = os.path.join(targetdir or tempfile.gettempdir(), ec_filename)
if os.path.exists(tweaked_spec):
if build_option('force'):
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)
Copy link
Member

@boegel boegel Oct 23, 2018

Choose a reason for hiding this comment

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

@mboisson Shouldn't we do this in dump itself instead? We may be using dump is other places in the future...

Copy link
Contributor Author

@mboisson mboisson Oct 23, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@boegel boegel Oct 23, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

@ocaisa ocaisa Oct 25, 2018

Choose a reason for hiding this comment

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

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
    """

Copy link
Member

@ocaisa ocaisa Oct 25, 2018

Choose a reason for hiding this comment

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

Perhaps doing a backup is sufficient? That way nothing ever gets removed and no need for the force option

Copy link
Contributor Author

@mboisson mboisson Oct 25, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@boegel boegel Oct 25, 2018

Choose a reason for hiding this comment

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

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).

Copy link
Member

@boegel boegel Oct 30, 2018

Choose a reason for hiding this comment

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

@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...

Copy link
Member

@ocaisa ocaisa Oct 31, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

@boegel boegel Nov 15, 2018

Choose a reason for hiding this comment

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

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

parsed_ec['ec'].dump(tweaked_spec)
_log.debug("Dumped easyconfig tweaked via --try-toolchain* to %s", tweaked_spec)

Expand Down