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

remove ancient/unused scripts #39

Merged
merged 6 commits into from Jun 20, 2017
Merged

remove ancient/unused scripts #39

merged 6 commits into from Jun 20, 2017

Conversation

jhoblitt
Copy link
Member

@jhoblitt jhoblitt commented Mar 30, 2017

Removes ancient/unused/broken scripts, all installation of files other then ./ups, a noop "doc build" and uneeded scons machinery. This converts this product essential into a table file for declaring variables and the SCM home for newinstall.sh.

@jhoblitt jhoblitt changed the title remove ancient/unused scripts + install of ./bin (empty) remove ancient/unused scripts Mar 30, 2017
@jhoblitt jhoblitt requested review from timj and ktlim March 30, 2017 01:14
@jhoblitt
Copy link
Member Author

This PR cuts a bit deeper than #38 but I believe all of the removed scripts are [long] out of disuse.

@RobertLuptonTheGood
Copy link
Member

Rather than remove the autoconf check for pre-requisites, wouldn't it have been better to fix it?

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

From my point of view I don't see why any of this stuff is in the package so I'm okay with it being deleted. I don't imagine that the autoconf stuff ever triggers in a normal build of this package. I'd like @ktlim to review this though as I'm not aware of all the history. This is the only package that is mandatory when newinstall.sh is executed so it would be nice to know why we even have scons involved if the package is now a newinstall.sh stub (scons forces sconsUtils and doxygen to be installed).

@@ -1,7 +1,6 @@
setupRequired(scons)
setupRequired(sconsUtils)

envPrepend(PATH, ${PRODUCT_DIR}/bin)
envPrepend(EUPS_PKGROOT, http://sw.lsstcorp.org/eupspkg, |)

# Deprecated: here for backwards compatibility, but discouraged from using
Copy link
Member

Choose a reason for hiding this comment

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

So what about these deprecated entries?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fairly confident that LSST_HOME from this package isn't being relied on but I'm not sure. Leaving it doesn't seem to hurt for the moment but I would certainly like to get rid of it.

@jhoblitt
Copy link
Member Author

@RobertLuptonTheGood The autoconf check isn't being used and is out of date, so we've obviously been living with out it. This package isn't at the bottom of a deep chain so I'm not sure how much good it would do anyways. If it was, it would almost certainly be problematic for products such as qserv as it would force unneeded dependencies. IMHO - probing the system for deps should be done on a per package basis where there is some hope of the probes being kept up to date.

@jhoblitt jhoblitt merged commit c32c6e6 into master Jun 20, 2017
@jhoblitt jhoblitt deleted the tickets/DM-9526-cleanup branch June 20, 2017 19:07
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

3 participants