add support for backing up modules with --module-only via --backup-modules #2134

damianam · 2017-02-27T13:28:06Z

With this PR easybuild will create a backup of existing modules when using --modules-only. 2 extra comments:

While I was at it, I also used the new back_up_file function to rotate logs
I think it makes sense to print the diff of modules in stdout. Thoughts?

boegel · 2017-04-26T16:01:41Z

@damianam This needs more work (e.g. review & tests), and since neither of us will have time to work on this in the coming days, let's bump the milestone to 3.3.0 on this and revisit it later.

damianam · 2017-06-13T14:51:53Z

@boegel take a look when you can. I'll see if tomorrow I can write some tests for it.

boegel · 2017-06-15T12:35:28Z

easybuild/framework/easyblock.py

+            if module_only and os.path.exists(mod_filepath):
+               warning_msg = "Old module file found. Backing it up in %s. Diff is:\n%s"
+               mod_bck_filepath = back_up_file(mod_filepath, backup_extension="bck")
+               (mod_diff, _) = run_cmd("diff %s %s" % (mod_bck_filepath, mod_filepath))


use diff -u?

boegel · 2017-06-15T12:36:08Z

easybuild/framework/easyblock.py

+            module_only = build_option('module_only')
+            if module_only and os.path.exists(mod_filepath):
+               warning_msg = "Old module file found. Backing it up in %s. Diff is:\n%s"
+               mod_bck_filepath = back_up_file(mod_filepath, backup_extension="bck")


we should make this a hidden module, no?

also, this should be configurable, people should be able to disable the backup if they're confident enough (but I guess having it enabled by default makes sense)

Making it a hidden module makes sense just for TCL modules. I had lua modules in mind, but since there are still people out there with TCL modules I agree with you.

Regarding making it configurable..... I am not so sure.

Pros:

Flexibility.

Cons:

Yet another option cluttering the --help

More code to maintain

However, if you are using TCL modules with Lmod, these will be recognized as modules that can be loaded, which is not what you want. Any other combination (TCL modules with Tmod, or lua modules with Lmod), will result in an invisible (to the modules tool) backup. So yes, I guess I should also prepare an option for it.....

boegel · 2017-06-15T12:37:27Z

easybuild/tools/filetools.py


+            # remove first to ensure portability


clarify portability?

boegel · 2017-06-15T12:39:16Z

easybuild/tools/filetools.py

+
+
+def back_up_file(src_file, backup_extension=""):
+    """Backs up a file appending a backup extension and a number to it. Returns the name of the backup"""


clarify: the number is optional (only when existing backups are already there)

use :param: to document arguments

also support hide named argument to backup to a hidden file

boegel · 2017-06-15T12:40:03Z

@damianam For functions you touch/change, please check whether they have a dedicated unit test already (and maybe enhance it while you're at it).

When adding new functions, add a dedicated test for them.

…arify stuff in comments.

damianam · 2017-07-14T15:25:33Z

@boegel I addressed all your comments. The code hasn't been tested yet, and the tests are still missing, but most of the bits and pieces should be there already.

…dule. Written full tests for that functionality.

damianam · 2017-07-19T13:19:15Z

Tests are complete and code seems to be working. @easybuilders/easybuild-framework-maintainers please review.

boegel · 2017-08-11T14:57:53Z

easybuild/tools/options.py

@@ -718,6 +720,10 @@ def postprocess(self):
            build_easyconfig_constants_dict()  # runs the easyconfig constants sanity check
            self._postprocess_list_avail()

+        # if --backup-modules is used without --module-only print a warning
+        if self.options.backup_modules and not self.options.module_only:
+            print_warning("--backup-modules can be used just together with --module-only. Ignoring it...")


I'd make this an error instead?

boegel · 2017-08-11T14:58:40Z

test/framework/filetools.py

@@ -494,6 +494,53 @@ def test_guess_patch_level(self):
        ]:
            self.assertEqual(ft.guess_patch_level([patched_file], self.test_buildpath), correct_patch_level)

+    def test_back_up_file(self):


missing additional tests for find_backup_name_candidate and move_file?

boegel · 2017-08-11T14:59:51Z

easybuild/framework/easyblock.py

        elif build_option('force') or build_option('rebuild'):
-            if os.path.exists(self.mod_filepath):
+            if os.path.exists(self.mod_filepath) and not build_option('backup_modules'):


rather than simply not removing, shouldn't we be renaming the module here already to its backup name?

boegel · 2017-08-11T15:01:12Z

easybuild/framework/easyblock.py

+                else:
+                    self.log.info(warning_msg + ' No differences found.', mod_bck_filepath)
+                    print_warning(warning_msg % mod_bck_filepath + ' No differences found.')
+                self.log.info("Module file %s written: %s", mod_filepath, txt)


this whole block is rather big to just shove it into make_module_step like this, it's better to move it into a separate method named back_up_existing_module_file or something like that?

boegel · 2017-08-11T15:01:31Z

easybuild/framework/easyblock.py

+                warning_msg = "Old module file found. Backing it up in %s."
+                hidden = False
+                if isinstance(self.module_generator, ModuleGeneratorTcl):
+                     hidden = True


indent is off here?

Also, why only do hidden for Tcl modules?

The backup should always be a hidden module imho, you don't want to expose the backup module to users?

Maybe we should also back up outside of the module tree, e.g. in the current directory (or at a location specified via --backup-modules?

Because lua modules have to end in .lua, otherwise they aren't treated as modules by Lmod, and don't show up with ml av. So there is no need to keep them hidden since the backup extension avoids that file ends in .lua.

Tcl modules on the other need to be hidden. Which is not that nice, since Lmod can still show them with --show-hidden. But that's the best compromise IMO. The alternative would be to remove the module header.

Backing up outside of the module tree kind of defeats the purpose of the backup. In my opinion the backup should be used for quick verification of the changes and for doing a "rollback" if necessary. If I have the current module and the backup in different files it slightly makes it more complicated and cumbersome, and more difficult to directly see if there was a previous version at any point with a simple ls

boegel · 2017-08-11T15:27:34Z

test/framework/toy_build.py

+        stderr = self.get_stderr()
+        self.mock_stderr(False)
+        self.assertTrue(os.path.exists(toy_mod))
+        self.assertTrue(os.path.exists(os.path.join(os.path.dirname(toy_mod), '.'+os.path.basename(toy_mod)+'.bck')))


use os.path.split & os.path.join, spaces around +

boegel · 2017-08-11T15:27:44Z

test/framework/toy_build.py

+        self.eb_main(args + ['--backup-modules'], do_build=True, raise_error=True, verbose=True)
+        stderr = self.get_stderr()
+        self.mock_stderr(False)
+        self.assertTrue(os.path.exists(os.path.join(os.path.dirname(toy_mod), '.'+os.path.basename(toy_mod)+'.bck_0')))


use os.path.split & os.path.join, spaces around +

boegel · 2017-08-11T15:28:05Z

test/framework/toy_build.py

+        warning = "WARNING: Old module file found. Backing it up in .* Diff is:"
+        diff = "some difference\n"
+        with open(toy_mod, "a") as fh:
+            fh.write(diff)


use write_file (it has an append mode)

boegel · 2017-08-11T15:29:11Z

test/framework/toy_build.py

+        ]
+        args = common_args + ['--module-syntax=Tcl']
+
+        warning = "WARNING: Old module file found. Backing it up in .* No differences found"


Hmm, should this be a warning? It's just an informative message, no?

boegel · 2017-08-11T15:31:37Z

test/framework/toy_build.py

+        if isinstance(self.modtool, Lmod):
+            args = common_args + ['--module-syntax=Lua']
+
+            toy_mod = os.path.join(self.test_installpath, 'modules', 'all', 'toy', '0.0-deps.lua')


rather than copy-pasting all the checks, we should hoist them in a helper function, and simplify the test to

def test_backup_modules(self): """Test use of --backup-modules.""" def run_checks(): ... ... toy_mod_fn = '0.0-deps' run_checks() if isinstance(self.modtool, Lmod): toy_mod_fn = '0.0-deps.lua' run_checks()

clean up back_up_file implementation & test

cleanup & tests for find_backup_name_candidate & move_file

…ncreasing number

reimplement find_backup_name_candidate to use timestamp rather than increasing number

fix broken move_logs test

…use diff_files function in make_module_step to print module file diff, trash useless module_generator.prepare method

…gure step to ensure identical module under --module-only

…s under --module-only & --skip + enhance/fix test_backup_modules

rework support for --backup-modules

boegel

lgtm

boegel · 2017-08-21T18:22:36Z

Going in, thanks @damianam!

Enabled backup of existing modules when using --module-only

f62855a

boegel modified the milestone: 3.2.0 Feb 28, 2017

boegel modified the milestones: 3.3.0, 3.2.0 Apr 26, 2017

damianam added 2 commits June 13, 2017 16:42

Added warning, clean up a bit the back_up_file function

75dacd6

Conflicts resolved

9949568

boegel requested changes Jun 15, 2017

View reviewed changes

boegel modified the milestones: 3.3.0, 3.4.0 Jun 22, 2017

damianam added enhancement priority:low labels Jul 12, 2017

Added hidden option to back_up_file. Added --backup-module option. Cl…

ab0d5c3

…arify stuff in comments.

Added test_back_up_file and fixed a bug when using hidden backups

733c9fb

damianam changed the title ~~Backup modules~~ (WIP) Backup modules Jul 18, 2017

Fixed code here and there that prevented the backup of an existing mo…

1b7ab7c

…dule. Written full tests for that functionality.

damianam changed the title ~~(WIP) Backup modules~~ Backup modules Jul 19, 2017

This was referenced Jul 19, 2017

Feature request: Backup of module files when using --module-only --force #2093

Closed

included --backup-module documentation easybuilders/easybuild#365

Merged

damianam added 2 commits July 20, 2017 17:48

sync

f9ae312

Make sure that the Lua tests happen just when Lmod is available

d86cd97

easybuilders deleted a comment from boegelbot Aug 11, 2017

boegel requested changes Aug 11, 2017

View reviewed changes

boegel mentioned this pull request Aug 17, 2017

clean up back_up_file implementation & test damianam/easybuild-framework#3

Merged

boegel and others added 8 commits August 17, 2017 10:42

fn_prefix & fn_suffix in back_up_file

f8d7624

Merge pull request #3 from boegel/backup_modules

8163498

clean up back_up_file implementation & test

remove target file first in move_file, add test for move_file

ab9addb

simplify find_backup_name_candidate & add test

cd4c395

Merge pull request #4 from boegel/backup_modules

03bad2a

cleanup & tests for find_backup_name_candidate & move_file

reimplement find_backup_name_candidate to use timestamp rather than i…

04dc98f

…ncreasing number

Merge pull request #5 from boegel/backup_modules

9f4d813

reimplement find_backup_name_candidate to use timestamp rather than increasing number

fix broken move_logs test

bbd07b2

damianam force-pushed the backup_modules branch from a6e052d to 9f4d813 Compare August 17, 2017 11:40

boegel changed the title ~~Backup modules~~ add support for backing up modules with --module-only via --backup-modules Aug 17, 2017

damianam and others added 7 commits August 17, 2017 13:55

Merge pull request #7 from boegel/backup_modules

e9acdf2

fix broken move_logs test

implement diff_files function in filetools module

74b0866

don't restrict --backup-modules to only --module-only

8ac188f

always create backup of existing module file under --backup-modules, …

bac286f

…use diff_files function in make_module_step to print module file diff, trash useless module_generator.prepare method

allow whitespace after 'files' lines in output of diff_lines

fb7ee56

minor style fix in options.py w.r.t. --backup-modules

61bb993

fix test for --backup-modules

3abe01a

boegel mentioned this pull request Aug 17, 2017

rework support for --backup-modules damianam/easybuild-framework#8

Merged

boegel and others added 4 commits August 18, 2017 14:35

fix typo in comment

b9a5560

define $TOY in test toy easyblock in constructor rather than in confi…

6f5bd4d

…gure step to ensure identical module under --module-only

use None as default for --backup-modules, auto-enable --backup-module…

ed86f96

…s under --module-only & --skip + enhance/fix test_backup_modules

Merge pull request #8 from boegel/backup_modules

b58d593

rework support for --backup-modules

easybuilders deleted a comment from boegelbot Aug 21, 2017

boegel approved these changes Aug 21, 2017

View reviewed changes

boegel merged commit 3d8bd2c into easybuilders:develop Aug 21, 2017

This was referenced Aug 23, 2017

add support for 'eb --inject-checksums' #2286

Merged

document --backup-modules easybuilders/easybuild#367

Merged

boegel mentioned this pull request Feb 3, 2021

add create_unused_dir function to create a directory which does not yet exist #3551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for backing up modules with --module-only via --backup-modules #2134

add support for backing up modules with --module-only via --backup-modules #2134

damianam commented Feb 27, 2017

boegel commented Apr 26, 2017

damianam commented Jun 13, 2017

boegel Jun 15, 2017

boegel Jun 15, 2017

damianam Jul 14, 2017

boegel Jun 15, 2017

boegel Jun 15, 2017

boegel commented Jun 15, 2017

damianam commented Jul 14, 2017

damianam commented Jul 19, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

damianam Aug 17, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel Aug 11, 2017

boegel left a comment

boegel commented Aug 21, 2017



		def back_up_file(src_file, backup_extension=""):
		"""Backs up a file appending a backup extension and a number to it. Returns the name of the backup"""

add support for backing up modules with --module-only via --backup-modules #2134

add support for backing up modules with --module-only via --backup-modules #2134

Conversation

damianam commented Feb 27, 2017

boegel commented Apr 26, 2017

damianam commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel commented Jun 15, 2017

damianam commented Jul 14, 2017

damianam commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel left a comment

Choose a reason for hiding this comment

boegel commented Aug 21, 2017