don't blindly overwrite an existing easyconfig in tweak_one

[boegel]

fix for bug reported by @Flamefire in #2503

[Flamefire]

What about the non-symetric handling of target_dir? In the case of no filename, target_dir is used (set to tmp-path if None) but if a filename is given it is not used.
Maybe the indentation is wrong and target_dir should always be used? (At least unless target_fn, despite its name` is absolute) This would also solve this.

It also doesn't help that target_dir is not documented. So what is its intended behaviour? The current usages seem to be with target_fn=None a target_dir set and target_fn set and target_dir=None. So I would guess that target_dir was intended to be used always. That makes the most sense (pass filename and path and auto-generate each if not set)

[boegel]

@Flamefire The correct implementation is what is intended: if target_fn is specified, the generated easyconfig file is created at the location it specifies (the _fn is indeed a misnomer, I'll fix that).

This is clearly mentioned in docstring for tweak_one:

Reads easyconfig file at path <src_fn>, and writes the tweaked easyconfig file to <target_fn>.

I'll clarify this by better documenting the arguments of tweak_one (especially targetdir) and by renaming src_fn and target_fn to better reflect they specify the (possibly relative) path to the location of the filename to read and the one to generate.

Thanks a lot for pointing this out, I appreciate thoughtful feedback like this a lot!

[Flamefire]

Much better. However at line 140 there is already a description of what happens if the filename is None. Maybe leave that paragraph as it was and just add the mentioning of target_dir to the paragraph at 140.

Although it is clear now I don't fully understand why it is written to the current working directory. One might not even have write-access to it (e.g. clusters). Writing it to a temporary directory makes more sense IMO.

[houndci-bot]

'easybuild.tools.filetools.remove_file' imported but unused

[houndci-bot]

'easybuild.tools.filetools.remove_file' imported but unused

[boegel]

@Flamefire Thanks for the suggestion, I updated the docstring for tweak_one accordingly. I've also enabled using --force to let tweak_one overwrite an existing file.

W.r.t. writing to the current directory: that's a design choice I guess, and it still makes sense to me... You usually run eb --try-* in a directory where you have write permissions, and letting eb generate the tweaked easyconfig directly where the user wants it is probably too much since there are different use cases there.

[Flamefire]

I find the following easier to understand:

Reads easyconfig file at path <orig_ec>, and writes the tweaked easyconfig file to <tweaked_ec>.

If <tweaked_ec> is not provided, a target filepath is generated based on <targetdir> and the
contents of the tweaked easyconfig file.

And:

:param targetdir: target directory for tweaked easyconfig file, defaults to temporary directory
                          Only used if tweaked_ec is None

Wrt current dir: Ok I kinda understand that in the context of try-xxx but for me this happened while just using eb --software xxx --toolchain yyy. So the generated EC was just a purely heuristic guess by EB and this kind of command may not be what you run in a writeable directory. EB simply failed to find the EC I intended to use. So here a permission error might confuse even more.

However if this logic is mangled with the try-xxx-Creation then its not worth to fix it. Just wanted to provide the context where I encountered this problem.

[migueldiascosta]

lgtm

[migueldiascosta]

lgtm