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

Generate relocatable loadLSST.* scripts #2

Closed
wants to merge 1 commit into from

Conversation

airnandez
Copy link
Contributor

The purpose of this proposed modification is to make newinstall.sh generate relocatable loadLSST.*sh scripts. This is a necessary (but not sufficient) ingredient for making the LSST software distribution relocatable.

If not already set, all the generated loadLSST.*sh scripts set at execution time the LSST_HOME variable to the directory where those shell-specific scripts are located. The current version of those scripts set that value to the directory where newinstall.sh was downloaded. The other components to bootstrap the installation (i.e. EUPS, Anaconda, git, etc.) are set up from the value of the LSST_HOME.

Please note that as of EUPS v1.3.0 the shell KSH is not supported since there is no KSH-specific setups.ksh for EUPS [see issue DM-380]. The loadLSST.ksh generated script takes that into account and terminates gracefully.

@RobertLuptonTheGood
Copy link
Member

Please note that as of EUPS v1.3.0 the shell KSH is not supported since there is no KSH-specific setups.ksh for EUPS [see issue DM-380]. The loadLSST.ksh generated script takes that into account and terminates gracefully.

@airnandez: I thought that the setups.sh file works with ksh. If it doesn't please file an issue detailing what goes wrong and I can add ksh to mksetups (which is how dash/zsh is handled as a variant of sh)

                        R

@@ -3,7 +3,7 @@
# **** This file should not be edited in place ****
# It is maintained in a repository at
# git@git.lsstcorp.org:LSST/DMS/devenv/lsst
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is just a whitespace change (a single whitespace at the end of the line got deleted). I've seen it elsewhere as well. Is your editor doing this automatically?

Could you fix that -- we prefer not to mix whitespace and functional code changes (see https://confluence.lsstcorp.org/display/LDMDG/Git+Commit+Best+Practices).

@mjuric
Copy link
Member

mjuric commented Oct 8, 2014

Final comment: could you squash all commits to a single one? We're trying to have one logical change per commit; having more than that (and especially merges), makes it difficult to read code history (and debug, if necessary).

Otherwise, a very good job (and definitely better than my first LSST commit :) ).

Generate shell scripts for setting up the minimal environment required to install LSST software
stack. Bootstrap scripts for the following shells are supported: bash, (t)csh, ksh, zsh. They
are named 'loadLSST.?sh' using an explicit extension to convey their intended use.

The bootstrap scripts allow for the LSST_HOME environment variable to point to a directory
where the LSST software stack is installed. If not already set when the bootstrap script is execute
it sets that variable to the location where the scripts are located and sets up the packages
the stack depends on.

NOTE: this is a combination of several commits to comply with LSST rules for pull requests.
@airnandez
Copy link
Contributor Author

Thanks for your comments. Here are mine:

  • My editor is set to strip trailing blanks, this is why there were whitespace changes. I changed that setting to comply to LSST practices.
  • I squashed several commits into a single one.

Please dismiss this pull request. I will submit a separate one which hopefully comply with LSST practices.

@airnandez airnandez closed this Oct 9, 2014
@PaulPrice
Copy link
Contributor

You can force-push over the top of this --- no need to submit a separate pull request.

@airnandez
Copy link
Contributor Author

@PaulPrice Thanks for your tip: I'm still learning the details on how git works and probably will never finish.

Anyway, I already submitted a separate pull request. Next time I will force-push.

@ktlim
Copy link
Contributor

ktlim commented Oct 9, 2014

FYI, we do want trailing blanks stripped, so don't change your editor configuration. I think Mario's point was just that there should be a separate commit that just does that (with appropriate commit comment), followed by the real feature commit.

@mjuric
Copy link
Member

mjuric commented Oct 9, 2014

@ktlim, @airnandez Yes, I'm sorry if that wasn't clear!

PS: @airnandez Yes, git has a pretty steep learning curve, and the first pull request is the hardest. But a few pull requests down the road you'll be an expert (we've seen this happen over and over with people).

Thanks!

@airnandez
Copy link
Contributor Author

Got it. Thanks for this clarification.

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

Successfully merging this pull request may close these issues.

None yet

5 participants