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.

Sign up for GitHub

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

Jump to bottom

First go at adding esutil build package #1

Merged
merged 1 commit into from Mar 31, 2016
Merged

First go at adding esutil build package #1

merged 1 commit into from Mar 31, 2016

Conversation

timj
Copy link
Member

@timj timj commented Mar 30, 2016

No description provided.

timj
timj reviewed Mar 30, 2016
View reviewed changes
README.md
@@ -1 +0,0 @@
# esutil
Copy link
Member Author

Choose a reason for hiding this comment

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

Can you put the README back (preferably with more text)? I imagine that needs TAP_PACKAGE=1 to go in an eupspkg.cfg.sh file.

@timj
Copy link
Member Author

timj commented Mar 30, 2016

How are we going to handle the problem that esutil has its own ups directory and completely overrides the table file that is included here and has different stuff in it? At the very least a comment needs to be made that the table file in the repository will be replaced.

@timj
Copy link
Member Author

timj commented Mar 30, 2016

Does esutil have any tests that I can run to see if it works?

@timj
Copy link
Member Author

timj commented Mar 30, 2016

esutil depends on scipy so we probably need to fix that when lsst-dm/legacy-scipy#1 is merged (assuming we can work out how to keep our stuff in the ups file).

@SimonKrughoff
Copy link
Contributor

I was pretty happy when it all just worked, but all these are good points. Do you have a suggestion regarding the ups directory in esutil?

@timj
Copy link
Member Author

timj commented Mar 30, 2016

Since patches are applied at prep time and the table file is over written at build time I think the best option is to have a patch file that rewrites setup.py to do what we want it to do.

@SimonKrughoff
Copy link
Contributor

There are no tests that I know of. Is that a blocker (even if this is meant to be temporary)?

@timj
Copy link
Member Author

timj commented Mar 30, 2016

It's not a blocker if upstream don't have tests. I just wanted to find out if my local build actually worked...

@timj
Copy link
Member Author

timj commented Mar 31, 2016

One complication is that esutil has its own table file that is copied in during the prep phase from the tar file. This replaces the version you have written. A patch file that solely prevents setup.py from overwriting the table file is too late.

@SimonKrughoff
Copy link
Contributor

@timj very sorry but I think I force pushed and erased your updates to the README.md.

If you are going to make more edits, give me a second, because I need to rebase again.

@SimonKrughoff
Copy link
Contributor

O.K. It actually works now. It didn't before, but I didn't notice because I had esutil installed in my local environment. I've checked and it imports and from the correct location.

I got around the table file problem by modifying the tarball in upstream to remove the esutil.table that ships with the package. I also added a patch that takes out the code to overwrite the table file. I hope that's sufficient.

@timj
Copy link
Member Author

timj commented Mar 31, 2016

@SimonKrughoff I don't remember pushing any changes to the readme...

@SimonKrughoff
Copy link
Contributor

O.K. I didn't confirm that it was you. I just saw some changes go by, but didn't realize the ramification until after I pushed.

@SimonKrughoff SimonKrughoff force-pushed the tickets/DM-5652 branch 2 times, most recently from 15979f6 to 843ec79 Compare March 31, 2016 17:50
@SimonKrughoff
Add esutil package
0a272f7 
The complication is that esutil ships with its own `ups/esutil.table` file.
It also overwrites the shipped version in `setup.py`.

We want to use our own custom table file.  To do this I did the following:
* made an override to `prep` to link the LSST version into place after the package
is unpacked.
* added a patch that removes the parts of `setup.py` that rewrite the file.

Note that we still need to include a link to the LSST table file in the repo, even though
it is overwritten, to setup the environment initially.
@SimonKrughoff SimonKrughoff merged commit 4723315 into master Mar 31, 2016
@ktlim ktlim deleted the tickets/DM-5652 branch August 25, 2018 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@timj @SimonKrughoff