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

Fixing UVCDAT runtime environment #747

Merged
merged 6 commits into from Feb 2, 2015

Conversation

Projects
None yet
6 participants
@remram44
Contributor

remram44 commented Oct 6, 2014

Sourcing the setup_environment.sh before using the software is error-prone. Furthermore, it is not actually needed anymore when running 'uvcdat', since the wrapper script will source it; this causes a warning to be displayed (UVCDAT setup already sourced for this install location.).

Also, once setup_runtime.sh is sourced, a whole bunch of executables appear in the path, replacing the system's default (things like python, curl, uuid... 260 in total on my system). For the same reasons, installing UV-CDAT globally is not a good idea (will break system's python).

This pull request intends to make the setup_runtime logic internal, and exposes uvcdat and uvcdatpython wrappers that can safely be added to the PATH (since they are on a directory of their own).

TODO:

  • Change setup_runtime.sh
    • Re-running the script for same installation path is fine
    • However running for a different path fails
    • Remove shebang, should only be sourced
  • Create wrappers
    • Scripts that set the environment then run their conterpart
    • 'cdat' (python), 'uvcdat' (vistrails), 'activate_uvcdat' (setup_runtime.sh')
    • They get the path from @-variables, not their location; they can be installed out of the install tree (e.g. somewhere on the user's PATH)
    • uvcdat.mac.in wasn't updated, I don't know what that is
  • Get CMake to configure and install wrappers
    • They get copied at build time or with make wrappers (if you need to do it as root)
    • Location configured by WRAPPER_INSTALL_LOCATION cache variable, defaults to ${CMAKE_INSTALL_PREFIX}/wrappers (not so useful but you can manually add on PATH)
    • make install cannot be used, it is broken because of its special use in CPack
  • Update setup_runtime.csh
    • I don't know much about csh, need help
# Everything beyond this point will be determined relatively
# from this path.
install_prefix="${UVCDAT_INSTALL_PREFIX:-@CMAKE_INSTALL_PREFIX@}"

This comment has been minimized.

@aashish24

aashish24 Oct 6, 2014

Contributor

I believe we will need UVCDAT_INSTALL_PREFIX for post-install script. Do we really need to take it out?

This comment has been minimized.

@remram44

remram44 Oct 6, 2014

Contributor

Are you sure it's used anywhere? A search for it came up empty... @CMAKE_INSTALL_PREFIX@ should always be correct anyway.

This comment has been minimized.

@aashish24

aashish24 Oct 6, 2014

Contributor

I believe the intention is that we will use that for the DMG and for any other post-install script. We cannot use CMAKE_INSTALL_PREFIX as it is used internally by the cpack. If it is not causing any issue for you, I would say leave it for now, and later we can look into it carefull.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Oct 6, 2014

@remram44 can you please rename uvcdatpython to "cdat" that what it used to be and lots of user actually came and asked me what happened to "cdat".
also we probably to need to update "cdscan" executable to use "cdat" rather than "python" now. i think cddump needs to be updated as well.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Oct 6, 2014

Renaming to cdat is 👍

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Oct 6, 2014

@remram44 one thing I should note that, are we making a release for 2.0 today, this change might go into master after that unless we can test this change (I cannot as I am busy with some other critical items but may be someone else can)

@remram44

This comment has been minimized.

Contributor

remram44 commented Oct 6, 2014

Yes, this is probably big enough to require more than a 1-day testing period.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Oct 6, 2014

Thanks @remram44 for the understanding. I am hoping that we can cut the release branch today

@remram44 remram44 added this to the 2.1 milestone Oct 6, 2014

@durack1

This comment has been minimized.

Member

durack1 commented Oct 6, 2014

Folks, there's a major issue here in that the setup_runtime has a .csh twin which it doesn't appear it also edited..

I would strongly recommend you folks at a tcsh/csh test case for this, as the bash-only testing is sure to miss configuration issues..

I'd be happy to test this out too..

@remram44

This comment has been minimized.

Contributor

remram44 commented Oct 6, 2014

there's a major issue here in that the setup_runtime has a .csh twin which it doesn't appear it also edited..

This branch is completely untested and not meant to be included in 2.0.

I'm not familiar with csh so somebody else will have to do it before merging; adding a task.

@jypeter

This comment has been minimized.

jypeter commented Oct 7, 2014

I confess I'm a tcsh user and I ran into some initialization problems this afternoon that almost got me crazy. I had a quick look at different issues referencing problems with setup_runtime.csh and I have just found out that my problem was due to my using an alias as a shortcut for the source!

I used the following test script

/bin/csh

# Find the location of this script and store it for further use
set sourced=($_)
echo "sourced=[${sourced}]"
set DIR=`dirname $sourced[2]`
echo "DIR=[${DIR}]"

# The end

And got the following results. Note the very different and wrong result I get when I try to use the alias

  >source /home/scratch01/jypeter/test_sourced.csh
sourced=[source /home/scratch01/jypeter/test_sourced.csh ]
DIR=[/home/scratch01/jypeter]

  >alias testinit 'source /home/scratch01/jypeter/test_sourced.csh'

  >alias testinit
source /home/scratch01/jypeter/test_sourced.csh

  >testinit
sourced=[testinit ]
dirname: missing operand
Try `dirname --help' for more information.
DIR=[]


@remram44

This comment has been minimized.

Contributor

remram44 commented Oct 7, 2014

This doesn't look like it has anything to do with this pull request -- as I said I didn't change anything csh-related.

@jypeter

This comment has been minimized.

jypeter commented Oct 7, 2014

Right, sorry! Don't know where I should list this problem/feature :( Maybe this should go to a "faq" or "askbot" page...

@durack1

This comment has been minimized.

Member

durack1 commented Oct 24, 2014

@remram44 I'd just like to add my perspective here.. Like a bunch of other users the way that I utilize the UV-CDAT functionality is on the command line, and I'd be surprised if any usage has ever triggered the (GUI?) wrapper script executing..

For this reason I generally run the setup_runtime.csh (yes I am a tcsher) in whichever terminal window I'm using UV-CDAT, and then happily execute the UV-CDAT python interactively, or calling python scripts to do my work.

I also quite like how after running setup_runtime.csh I get access to all the more recent binaries associated with the UV-CDAT install, ncdump 4.3.x etc etc..

So I would certainly push back against where I think you're suggesting this configuration should go.. I agree what you propose is great for GUI instances, but not for users who use the command line functionality like I do..

@doutriaux1 will also likely have a strong opinion on this too..

@remram44

This comment has been minimized.

Contributor

remram44 commented Oct 24, 2014

I see. It is certainly interesting to have some feedback here. My goal was to have a wrapper ('uvcdat') for the GUI and another ('uvcdatpython' or 'cdat' as requested by @doutriaux1) for Python, so you'll still be able to run Python scripts or interactive Python sessions easily; however I did not envision that there might be other common entry points.

Of course, setup_runtime.sh stays available. It just wouldn't be necessary for the "uvcdat" and "cdat" commands I wrapped.

@durack1

This comment has been minimized.

Member

durack1 commented Oct 24, 2014

@remram44 I've pulled my hair out trying to get the current version of setup_runtime.csh working across all the use cases that I encounter frequently.. Like @jypeter I've also hit very weird config instances/errors by adding a source line to my .cshrc - I do think this should all be cleaned up, and having to support two separate scripts seems like a very painful (and error/bug prone) way to proceed.. At the very minimum I'd suggest that these config scripts get added to the test suite, for both a bash and tcsh/csh environment, so that problems are identified prior to software being compiled and available for users to download..

I'd be more than happy to help out with the testing..

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Oct 27, 2014

@remram44 i think the "cdat" road is a good one we used to have this and users seemed to like it (I still have users coming in my office asking about where it went). BUT will it solve the "multiple cdat in your system" issue. Our users (and developpers) switch back and forth between many versions of UV-CDAT, do you think adding the path to "cdat" in their .login/.profile (to be shell agnostic) will work and the can still use another "cdat" executable?

@jypeter

This comment has been minimized.

jypeter commented Oct 27, 2014

Yep, a wrapper for the uvcdat GUI, another one for python, and a script to initialize the full environment for those who need it seem like a good way to go. And we just have to minimize the number of places where we have to hard-code the actual location of the full installation

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Oct 27, 2014

I would prefer we have a real executable vs a python script if we go that route. ParaView does that for desktop apps that pulls all the libraries and python path into it which eliminates setting up the paths for the actual app. It will also fix the issue for Mac DMG (and run time controls as well)

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 21, 2015

@doutriaux1 I have to review this branch. I am wondering if you reviewed it already?

  • Aashish
@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 21, 2015

This is definitely not ready, I'll try and do one more pass on this asap.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 21, 2015

Great. It would be nice if you can layout a roadmap here (as serveral people made suggestions after your initial commit). Something as simple as bulleted list, nothing too fancy.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 21, 2015

as far as I can tell the major issue with it was that it fixes ONLY .sh we cannot bring it in until .csh is fixed as well (or at least confirm the issue does not exists for .csh)

@durack1

This comment has been minimized.

Member

durack1 commented Jan 21, 2015

Would it be possible to add csh tests to duplicate any tests that run on the sh versions? It seems kinda silly to have this environment test omitted when it's an important initialization script..

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 21, 2015

I think I'm done, apart from the CSH script. I updated the todo-list.

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 21, 2015

I have no idea what make install does currently, apart from installing my wrappers. Here it made a "UVCDAT" directory besides "install", that looks to be a copy, yet doesn't work if "install" is removed; can anyone explain?

@painter1

This comment has been minimized.

Contributor

painter1 commented Jan 21, 2015

At least for the diagnostics, "make install" makes two copies, one in a local build directory and the other under the Python's site-packages directory.  The one which really matters is under site-packages.  I think that a UV-CDAT build is similar, but more complicated.

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 22, 2015

I think UV-CDAT's build just installs everything during the make phase and the make install is completely broken, but I'd like confirmation before I work around this issue.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Jan 22, 2015

@remram44 you're right the make install is completely ignored at the moment. "make" does everything, I "think" it's because some packages needs other to be installed in place before build starts, @aashish24 we could probably revise that? But that would be a lot of work for not much benefit (and it would add one line to the install instructions) but at least it would be consistent with regular build systems.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 28, 2015

we should't be installing to the system directories. We install things where the CMAKE_INSTALL_PREFIX is set. Please use that.

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 28, 2015

Here it is again: the point of this branch is to have "cdat" and "uvcdat" wrappers, together with "setup_runtime", in separate directories that can be safely exposed to the user without bringing in all the UV-CDAT environment which would definitely break the user's.
If I'm putting these in install/bin this effort has no point whatsoever.

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 28, 2015

@remi one thing you could do is define a new variable WRAPPER_INSTALL_PREFIX (which can defualt to user's home directory or something reasonable). If you do that, and define this as a external target, then someone can just do this:

make wrapper (call it something else )

to me, this makes much more sense. Also, some users might have not root permissions (often in a control environment). So installing it at /usr should n't be the default IMO. Let me know what you think of it. Thanks

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 29, 2015

The path is not exactly a prefix, since I don't append eg. /bin at the end. The variable is the actual directory into which the wrappers are put. It is currently named WRAPPER_BINARIES_LOCATION.
The users only need root permissions if that variable is set to a location they can't write to (which is not the default). They can also copy/move them anywhere, they won't break. But it's tedious IMHO if there is a make target that only does that.
No other option I suppose, if CPack cannot be tamed. I'll do that soon.

There is a possibility of having the activate_uvcdat script load in a deactivate command, the same way Python's virtualenv system work. Preliminary work (for bash only, I'm afraid I still don't know anything about csh) is here but it needs unit tests. Is that of interest?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 29, 2015

  • WRAPPER_BINARIES_LOCATION sounds good to me. You can also think of more conventional name

WRAPPER_INSTALL_LOCATION

  • Right, the root permission issue is independent of whether or not we install it during the make or make install time
  • I am less willing to change the CPack stuff (that is already complicated)
  • activate_uvcdat and deactivate would be nice!! I would have something short though (loadcdat unloadcdat)

remram44 added some commits Oct 6, 2014

Fixes setup_runtime script
The script should really only use the current installation (not
UVCDAT_INSTALL_PREFIX from the environment).

Re-running the script is not an error (return 0), however displays a
warning because this happens when users source setup_runtime manually
before running 'uvcdat'.
Removes shebangs from setup scripts
These are not meant to be run, only sourced.

@remram44 remram44 force-pushed the remram44:setup-environment branch from 9956c47 to f6076ee Jan 30, 2015

Adds 'wrappers' target, installs wrapper scripts
This target, run by default, installs wrapper scripts "uvcdat", "cdat"
and "loadcdat" into WRAPPER_INSTALL_LOCATION (by default
install/wrappers). These scripts setup the environment, so they are safe
to be brought in the user's PATH; they won't conflict or break the
environment.

This should really be "make install" but that target is broken by our
CPack setup.

@remram44 remram44 force-pushed the remram44:setup-environment branch from f6076ee to 4b03f91 Jan 30, 2015

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 30, 2015

Alright, I rebased everything. Changes:

  • WRAPPER_BINARIES_LOCATION -> WRAPPER_INSTALL_LOCATION
  • Wrappers installed by the wrappers target (e.g. make wrappers), run by default (e.g. make or make all)
    • They can still be copied from install/wrappers/ to somewhere else if you didn't change WRAPPER_INSTALL_LOCATION, they will run from anywhere
  • Renamed activate_uvcdat to loadcdat

@remram44 remram44 modified the milestones: 2.1.1, 2.1 Jan 30, 2015

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Jan 30, 2015

@remram44 thanks. Much appreciated. I will review it and get it merged today.

@remram44

This comment has been minimized.

Contributor

remram44 commented Jan 30, 2015

Note (cf roadmap) that uvcdat.mac.in and setup_runtime.csh were not updated.

@@ -39,6 +35,10 @@ library_paths=( @SETUP_LIBRARY_PATHS@ )
python_paths=( @SETUP_PYTHON_PATHS@ )
executable_paths=( @SETUP_EXECUTABLE_PATHS@ )
if [ -z "$UVCDAT_DISABLE_PROMPT" ] ; then

This comment has been minimized.

@aashish24

aashish24 Feb 2, 2015

Contributor

we should document it somewhere...

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 2, 2015

LGTM 👍 except the documentation part.

aashish24 added a commit that referenced this pull request Feb 2, 2015

Merge pull request #747 from remram44/setup-environment
Fixing UVCDAT runtime environment

@aashish24 aashish24 merged commit ae90d4a into CDAT:master Feb 2, 2015

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@durack1

This comment has been minimized.

Member

durack1 commented Feb 3, 2015

@aashish24 @doutriaux1 just wondering whether this should be closed when the *.csh variant hasn't been updated?

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 3, 2015

How soon you can update the CSH variant?

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 4, 2015

@aashish24 I push a PR to "undo" the prompt change. I understand the idea, but it should be done right i.e

  • give which version of uvcdat you're using
  • maybe last part of the directory of install
  • make it prettier than this

In the mean time I turn this ON only if requested, having like that by default is just not right it think

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 4, 2015

also why did we approve this before csh is done. We need to keep these both in sync @durack1 and @jypeter are not the only csh users out there. We can't just blow them away because we don't like tcsh (nothing worng with not liking tcsh but we have to support it anyway 😉 )

@aashish24

This comment has been minimized.

Contributor

aashish24 commented Feb 4, 2015

The wrappers won't break the existing means to run uvcdat right? As far as we can don't break the current functionality I think it's okay to add new one. Thoughts? We won't annouce the feature until then. Also if remi can push a fix for csh soon then also this should be fine.

@remram44

This comment has been minimized.

Contributor

remram44 commented Feb 4, 2015

Oh, I didn't intend to update the CSH version myself... If a CSH user could do it, that would be a lot faster (and probably safer) than if I have to learn about CSH first.

@doutriaux1

This comment has been minimized.

Member

doutriaux1 commented Feb 4, 2015

@durack1 or @jypeter it would be awesome if you could help on this end. I could do it but it's going to take me weeks/months before I can get to it.

@remram44 remram44 referenced this pull request Feb 24, 2015

Merged

Removes a warning #1069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment