-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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.
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
Add two options diy #2771
Add two options diy #2771
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Couple of minor change requests.
lib/spack/spack/cmd/diy.py
Outdated
@@ -91,13 +101,21 @@ def diy(self, args): | |||
tty.msg("Uninstall or try adding a version suffix for this DIY build.") | |||
sys.exit(1) | |||
|
|||
source_path = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't actually need this line -- ifs in Python don't have their own scope.
lib/spack/spack/cmd/diy.py
Outdated
'-j', '--jobs', action='store', type=int, | ||
help="Explicitly set number of make jobs. Default is #cpus.") | ||
subparser.add_argument( | ||
'-d', '--source-path', dest='source_path', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can add default=None
here to simplify the logic below...
lib/spack/spack/cmd/diy.py
Outdated
@@ -91,13 +101,21 @@ def diy(self, args): | |||
tty.msg("Uninstall or try adding a version suffix for this DIY build.") | |||
sys.exit(1) | |||
|
|||
source_path = None | |||
if args.source_path is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be shortened to:
source_path = args.source_path
if source_path is None:
source_path = os.getcwd()
@@ -62,6 +68,10 @@ def diy(self, args): | |||
if not args.spec: | |||
tty.die("spack diy requires a package spec argument.") | |||
|
|||
if args.jobs is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a common argument with install
, can you factor this and arg from install
into spack.cmd.common.arguments
? Look at the find
, install
, module
, setup
and spec
commands do it for examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such factoring has already been done (extensively) in #2664. Please base your changes off of that PR.
a1681f8
to
ebff7c6
Compare
@tgamblin I've incorporated your suggested changes. -j is now a common argument and the source_path logic has been simplified. Actually, I've started to wonder why diy is a separate command from install at all. An option to pass a source directory could be added to install and the functionality of diy would be duplicated as far as I can tell. diy only seems to create a DIYStage object with the path of the source directory. The user is still in charge of picking a spec for that build though. thoughts? |
ebff7c6
to
e1451ee
Compare
@krafczyk Please see #2664. It suggests plans to re-do The changes to
This would install A and C "normally" while doing a These changes and functionality are already working for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see the ideas here integrated into #2664 and submitted together as one PR.
@citibeth: this PR is a relatively small change, and I think #2664 is a much larger one that I haven't had a chance to look at. I would favor accepting this and rebasing #2664 on top of it for now. Especially the Also, IMHO the semantics of DIY are very different from those of All that said, I do really like the multi-setup stuff in #2664, but I wonder if it would be better implemented as a project file, and not just command line options to |
Just to give you guys a bit of perspective of where I'm coming from, I created a small python script a while back which assisted me in building and installing software from source here: https://bitbucket.org/krafczyk/buildpkg Essentially I used it as a kind of adapter to unify the build methods and arguments from several different build systems (cmake and autotools only at the moment). It seemed to me that there were 3 locations that the user could want mutable.
spack already takes care of 1. in the normal mode, but provides the user the opportunity to define it with DIY builds. 2. is similar to but not the same as the stage directory. This is because it may be desirable to do an out of tree build for some packages. This is especially important for DIY builds since build files may pollute the source directory without this option. 3. is of course already managed by spack, so no need to change this I would think, but maybe the odd user would want the option to set the install directory anyway. I can see this being accomplished with The install process in spack feels like it should be essentially the same except it does magic to manage all the packages for you. This amounts to automatically finding source, managing the build directory and managing install directories and keeping track of what is and is not installed. Then, the only differences between my
To me,
@tgamblin: I certainly agree the semantics of So, to me the bitrot is quite unnecessary. Everything ought to go in one command You could build This would also allow the possibility of requesting multiple packages at the same time from install and essentially joining their DAGs where possible to create a consistent runtime environment.
Finally, to tack on the end, spack should allow the ability to do out of tree builds especially for DIY style building. This is useful especially for large projects with long build times where you may need to rebuild to test for new features and dont' want the whole build process to run each time. It would look something like this:
|
@krafczyk: That sounds like a pretty good breakdown of the problem to me. Thoughts I had while reading this:
I agree that these are all different ways to tweak the build pipeline. You may have already seen that
I think I like the fact that the syntaxes you and @citibeth are proposing let the user pick the parts of a build they want to work on. Worst case we can probably add the args you're suggesting to
There are a lot of packages that do "out of tree" builds in the sense that they make a separate directory (grep for That said, I think it would be great to try to make most packages build out of tree by default, and to figure out how to modify the package API to support that. The issues I see are:
I think packages will end up having to say whether or not they support out of tree builds. But if we do this, then we can have packages do things like create build stages with consistent names -- most packags use Thoughts? |
Well... yes and no. Or to put it more formally... if
then
It then leaves the user to run the returned One particularly unusual thing about
We should probably move
The initial motivating factor of #2664 was that More recently, I decided I need a
I agree, the current syntax isn't really thought through. But I'm not inclined to think more deeply about that UI issue. If someone has an alternate syntax that they like better, I'm happy to use it.
This isn't just a nice byproduct of the syntax; it is a necessary feature that any syntax needs to support.
Yes, that sounds like a logical feature. But not sure I really need it or would use it. I would want someone who REALLY WANTS it, then we know that whether implementation of it actually solves ONE person's problem.
Not any harder than cramming a zillion variants and OTOH... I agree, some kind of "project setup" file would be nice. Then we just give the project setup file on the command line. How about if we do it this way:
With these three steps, I could create a "project" file that tells Spack (for example) which packages I intend to setup vs. DIY vs. regular-build. Allowing YAML to proliferate like this could be powerful. Of course, not all YAML options or config files would be appropriate in a "permanent" configuraiton (such as in Your thoughts on this?
#2664 already did that. I suppose, then, that #2664 is worst case.
If you want to build a (CMake) package many times for one source directory, just do I use this feature of It currently only works with CMake. But there's no reason it can't work with any package, someone just has to write the appropriate build phase. |
You should be good to go.
I think it's best to make all the changes in #2664 On UI... I'm coming to the conclusion that we should configure which packages are built with install vs diy vs setup in a YAML file. When combined with #2686 this will allow us to create configurations files, on a per-project basis, that we include on the command line as needed. This change will probably happen along with our overall review of Spack environments. In the meantime, we have the current command-line interface. I would recommend just doing |
e1451ee
to
dcab47c
Compare
Also add -j to the common arguments
dcab47c
to
8a22045
Compare
@krafczyk I was using this PR to specify the package source folder with --source-path. |
@luigi-calori Originally we were going to add this in with #2664. However that one was never merged, and a new PR was created (#5043) which was supposed to be a 'second attempt' but that hasn't been finished either. I'll actually revive this one because its such a small change, and I don't want to wait for work to be finished on those branches any more. Thanks for reminding me! |
Superseded by #5963 |
Fixes #2573.
Add -j to diy which accepts the number of jobs to use for building
Add -d which accepts the source path to use for diy building