Skip to content
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

flux command should be runnable in build tree from any directory #14

Closed
grondo opened this issue Sep 30, 2014 · 9 comments
Closed

flux command should be runnable in build tree from any directory #14

grondo opened this issue Sep 30, 2014 · 9 comments

Comments

@grondo
Copy link
Contributor

grondo commented Sep 30, 2014

We should be able to use flux(1) cmd in build tree from any directory, e.g.

./src/cmd/flux start --size=2

flux run in this way should work the same as when run from src/cmd directly, i.e. it should still find all modules from builddir, point cmd dir to src/cmd, etc.

From top-level build directory. This will be necessary for running system testing.

@garlick
Copy link
Member

garlick commented Oct 1, 2014

What about

TESTS_ENVIRONMENT = \
    FLUX_TOP_BUILDDIR=$(top_builddir)

and flux using FLUX_TOP_BUILDDIR to prepend directories to its command/module search paths? This would work in flux-core or flux-whatever (using installed flux util).

So for example in a flux-sched project, flux would find flux-submit and could load schedsrv or whatever in tests while still finding all the installed flux-core bits?

@garlick
Copy link
Member

garlick commented Oct 1, 2014

Here's a set of commits I was working on:
garlick/flux-core@99399b9
garlick/flux-core@82b7e8f
garlick/flux-core@116045ce
garlick/flux-core@f357f762
garlick/flux-core@a81aac7a
garlick/flux-core@368c0b08
(Hmm, maybe I should have just made a pull request? But this isn't ready...).

@grondo
Copy link
Contributor Author

grondo commented Oct 1, 2014

This might work, but I'm concerned it is a bit ad hoc and having code in installed flux command that checks BUILDDIR and SOURCEDIR seems wrong somehow. Also it would be nice if you could chain together a series of flux projects and have ./flux-core/src/cmd/flux find all their stuff.

If it were me I'd gravitate toward a solution with colon-separated paths for flux-module-path and flux-command-path, perhaps with a default base path made up of

  • installed paths for installed version of flux
  • builddir paths for in-tree flux (with srcdir as well for flux-command-path)

Ideally adding new command and module paths could be done with something as simple as

FLUX_MODULE_PATH=$(pwd):${FLUX_MODULE_PATH}
FLUX_COMMAND_PATH=$(pwd):${FLUX_MODULE_PATH}

Maybe this could be extended via a config file which could be found in ${top_builddir} for
in-tree flux and ${sysconfdir} (or similar) for installed flux. The config file could be fronted
by a flux config command, and could also have per-user defaults under ~/.flux.

The main benefit I guess is that module-path and command-path make sense for both
installed flux and build-tree flux, and you can also chain as many directories to the
search paths as you need to cobble together various projects.

However, I do also get what you're doing by trying to make this easier and have only one
variable to set for the end user. And I may have missed some obvious problems with the
above approach.

@garlick
Copy link
Member

garlick commented Oct 1, 2014

What is the best way to communicate $(top_builddir) / $(top_srcdir) to flux.c do you think?
As far as I can tell your comments align with where I was going except for that.
The config file's a good idea as opposed to hardwiring the build tree structure in code!

@garlick
Copy link
Member

garlick commented Oct 1, 2014

Yeah... the more I think about it I really like the flux config idea.
Would it be OK do you think if tests passed flux the location of an in-tree config file, e.g.
flux -c $(top_srcdir)/flux.conf ....

Am I just having a hard time letting that concept go? :-)

@garlick
Copy link
Member

garlick commented Oct 2, 2014

I've had a go at the config file idea here: garlick@flux-config2

  1. implemented flux_config_load(), flux_config_save()
    ZPL format, ~/flux/.config by default, overrideable.
    These functions may need to be generalized a bit - they worked well for what I needed them for (flux-config.c, flux.c)

  2. new command flux-config

  3. flux now reads config and munges args and envirnoment quite differently.
    see garlick/flux-core@6d40802d7 comment for a summary.
    If /proc/self/exe indicates, load config from $(self_dir)/../../flux.conf

  4. generate $(top_srcdir)/flux.conf

Let me know what you think.

@grondo
Copy link
Contributor Author

grondo commented Oct 2, 2014

This looks like a great improvement to me!
My only comments are:

  • Do you think ~/.flux/config actually belongs in ~/.flux/cmd/config or similar to differentiate it from some other more general flux config we might have in the future? Or will this config serve all purposes and users just read the correct section? (Could this result in huge files if so?)
  • Similarly, should the "general" section perhaps be renamed "cmd" (I haven't thought this through, and maybe those paths will apply generally, but for now it seems they just apply to the flux command itself)?

I don't feel strongly about either point above, but they are things that came to mind when looking at the functionality.

Finally, for developers, it might be really useful to have an append and delete operation to operate on these paths without having to pick apart the quite long
values. (append and prepend would add to existing config entry,
delete would remove one matching path) This isn't important though and
could go in a feature request.

Also, I had failure to compile with gcc 4.8.2 on this branch:

make[3]: Entering directory `/home/grondo/flux-core/src/common/libflux'
  CC       fconfig.lo
fconfig.c: In function 'flux_config_save':
fconfig.c:87:9: error: format not a string literal and no format arguments [-Werror=format-security]
         err_exit (path);
         ^

garlick added a commit to garlick/flux-core that referenced this issue Oct 2, 2014
@garlick garlick mentioned this issue Oct 2, 2014
@garlick
Copy link
Member

garlick commented Oct 2, 2014

I fixed the gcc error with garlick/flux-core@d28a306 (untested).

I don't feel strongly about the others either - maybe we should press ahead and fix later after we've had a bit of experience with it?

I submitted pull request #18.

@grondo
Copy link
Contributor Author

grondo commented Oct 2, 2014

Agreed. I merged your pull request. (I had test the same fix as in garlick/flux-core@d28a306)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants