Modify file layout to conform to Spack prescriptions#5
Modify file layout to conform to Spack prescriptions#5alalazo wants to merge 2 commits intoFNALssi:masterfrom alalazo:fixes/spack_dev_works_with_current_develop
Conversation
Functionality might need further modifications. A few commands seem still work in progress.
dev/cmd/dev.py
Outdated
| '..', '..', 'lib')) | ||
| sys.path.insert(0, SPACKDEV_LIB) | ||
| # Import sub-commands | ||
| from ..subcommands import build_env, findext, getdeps, info, init, stage |
There was a problem hiding this comment.
@alalazo I spoke with @chissg today and I think the thing I missed was that he didn't want to have to refer to the commands explicitly: the idea is that he could add more subcommands and those could be imported here without updating this code. Perhaps the way to reconcile this with spack/spack#11145 is to update the commands in that PR to do the appropriate sys.path manipulations automatically.
The other changes in this PR (in the other files) relating to following the directory structure conventions that currently exist in Spack command extensions are agreeable.
There was a problem hiding this comment.
@scheibelp @chissg If the other changes are agreeable and don't prevent functionality to be implemented in SpackDev, this PR shows that we can reduce dev.py from 41 lines to 26 lines without a single line change in Spack. That brings me to the following two questions:
- How many subcommands do we need to add before editing the two lines above becomes a real maintenance issue?
- Is it worth to modify ~280 lines in Spack (with a ~100 lines addition) and add another 15 here just to avoid writing the import above?
Answering these two questions led to my latest review of spack/spack#11145.
There was a problem hiding this comment.
Perhaps the way to reconcile this with spack/spack#11145 is to update the commands in that PR to do the appropriate sys.path manipulations automatically.
The current mechanism carefully crafts modules with the appropriate __path__ and adds them to sys.modules to give extensions the best possible isolation. This is done here. How is adding paths to sys.path improving encapsulation over the current state when the new mechanism resorts to using a global variable that potentially affects every import statement in the program?
There was a problem hiding this comment.
@alalazo @scheibelp Please see spack/spack#11145 (comment).
There was a problem hiding this comment.
@chissg I'll read through the comment you linked, thank you. I'd appreciate to know which answers you would give to the questions I posted above.
| def import_subcommands_automatically(): | ||
| subcommand_glob = os.path.join(os.path.dirname(__file__), '..', 'subcommands', '*.py') | ||
| command_files = glob.glob(subcommand_glob) | ||
| subcommands.extend(filter( | ||
| lambda x: not x.startswith('__'), [os.path.basename(x)[:-3] for x in command_files] | ||
| )) | ||
| t = __import__('subcommands', globals(), locals(), subcommands, 2) | ||
| for name in subcommands: | ||
| globals()[name] = getattr(t, name) | ||
|
|
||
| import_subcommands_automatically() |
There was a problem hiding this comment.
I spoke with @chissg today and I think the thing I missed was that he didn't want to have to refer to the commands explicitly: the idea is that he could add more subcommands and those could be imported here without updating this code.
@chissg The change above permits to do what you ask for without changes to Spack develop. If you implement another subcommand in the subcommands folder then dev.py does not need any change to import it and make it available. I personally prefer the previous version with the explicit import, but this is just to demonstrate that adding this capability to an extension doesn't need any change to current Spack.
| module = spack.cmd.get_module_from(pname, 'fnal.spack.dev') | ||
| sp = subparser.add_parser(subcmd, help=module.description) | ||
| module.setup_parser(sp) |
There was a problem hiding this comment.
@chissg I'm re-reading the description of spack/spack#11145:
Enable the encapsulation of sub-commands to top level Spack or extension commands, removing the necessity to explicitly add sub-command parser and implementation invocations to the top level command.
Please note that this is true also for the case on the right, showing what can be done right now without modifications to Spack - so I think enable would not be the correct term. Can you please update the description or comment if you disagree?
3 participants
This PR rearranges the layout of Python files in SpackDev to follow the prescriptions Spack gives about custom extensions. It goes as far as making the subcommands load correctly and show their help. Minor fixes are still needed to get the full functionality - I'll be happy to proceed with this if the PR is considered helpful.
Using the layout that Spack prescribes, instead of the custom layout now used by SpackDev, this PR removes the necessity to tweak
sys.pathand reduces the lines of code in the maindevcommand.Subcommands are still kept in a separate folder, factored in the same way they are now.
To add a new subcommand it is only necessary to add its name in the import statement and in the list at the line below.New subcommands are added automatically if their implementation resides in thesubcommandsfolder.In the end this PR would make SpackDev work as illustrated in spack/spack/11145 without requiring any modification to Spack itself and
reducingnot augmenting the boilerplate needed to load subcommands in the maindevcommand.