Bill-Gray / find_orb

Orbit determination from observations
https://www.projectpluto.com/find_orb.htm
GNU General Public License v2.0
98 stars 45 forks source link

conda-forge compatibility updates #29

Closed mjuric closed 3 years ago

mjuric commented 3 years ago

Hi @Bill-Gray:

This is the conda compatibility patch for find_orb, to make it possible to distribute with conda-forge (i.e., Linux and Mac users could then get it running on their systems with something as easy as conda install findorb).

The key changes here:

With these changes, I was able to build portable conda binaries for find_orb, that seem to work as expected.

The details of other changes are in the commit messages. Take a look when you get a chance and let me know if you're concerned about any -- happy to try to make this better! Also, I tried to test the build in various conditions, but do let me know if I accidentally introduced unwanted build regressions (that's always easy to happen!).

mjuric commented 3 years ago

N.b., in case you're curious how all these changes get used to build a conda package, here is the "recipe":

https://github.com/B612-Asteroid-Institute/staged-recipes/tree/add_findorb/recipes/findorb

The most important files are meta.yaml -- the file that lays out the general settings (like the name, where to find the sources, etc.), build.sh -- the file with commands that build the package, and run_tests.sh -- the file that runs the tests to verify the package has successfully been built.

Once everything is merged, this recipe will get merged into the main conda-forge repository. Then they'll build the code for us automatically and make it available via conda install. I already have a work-in-progress PR out there.

Bill-Gray commented 3 years ago

Thank you; this looks quite good. A couple of comments: -- The -lncursesw vs. -lncurses bit has been an annoyance for some time. Ideally, we'd just say "if ncursesw is available, use that; otherwise, use ncurses." I just pushed commit e6b7b523c4a8e3f4204e6 to implement that. (Still enables you to override CURSES_LIB, as in your implementation.) -- I was a little puzzled at first as to why you'd be copying files around, until I saw the "HACK" and "Going forward it'd be good to rework Find_Orb to search for data in default directories if local copies don't exist" comments. Just so I'm clear on this : you'd like to be able to set up Find_Orb so that it'll look in ~/.find_orb (as it currently does), but if that doesn't work, it'll look in $(PREFIX)/share/findorb/data instead. This would have basically the same effect as the current version, but would not require copying files (everybody might use the same JPL ephemeris file, for example, stored in common for all users in $(PREFIX)/share/findorb/data).

Their access to the $(PREFIX)etc. directory would be read-only. If they modified files, copies would be written to ~/.find_orb.

Seems reasonable to me. But it may work a lot more cleanly to just have Find_Orb say "if you can't get something from ~/.find_orb, and you're reading rather than writing to the file, try getting it from $(PREFIX)etc." If that's what you'd prefer, let me know. I can see non-Conda use cases where multiple users could share most Find_Orb files in this manner, and I think it's a few-line change.

Bill-Gray commented 3 years ago

Should mention: if Find_Orb simply searches the $(PREFIX)etc. directory, that helps nicely if a new JPL ephemeris file or updated list of observatory codes becomes available. You just update the $(PREFIX)etc. version, and all users get it. With a "copying" scheme, you've got to copy the updates to each user. Or each user has to do the copying.

mjuric commented 3 years ago

@Bill-Gray Thanks for the notes! Regarding the copying, you're 100% right this is a hack (I tried to make this first patch minimally invasive).

The cleanest behavior (IMHO) might be to have a file lookup hierarchy (for reading) such as:

  1. search current directory, if not exists ...
  2. search ~/find_orb, if not exists ...
  3. search $PREFIX/share/...

(which I think is similar to what you're saying). I'd also propose for writes to happen only into the current working directory (or some user specified directory), like (I think) it's on Windows right now. That way it's possible to avoid issues with locking, and one can run multiple simultaneously find_orbs (useful on multi-core machines where we use it for "industrial" fitting of hundreds of thousands of orbits).

PS: For the ncurses change, I'll leave a comment on the commit -- it's a good idea, but may be brittle as compiler error messages aren't standardized.

mjuric commented 3 years ago

N.b, I rebased this PR to your master's HEAD, so it should be possible to cleanly merge.

I tried to fix the issue w. clang in your commit for curses autodetection-- see https://github.com/Bill-Gray/find_orb/pull/29/files#diff-beda42571c095172ab63437d050612a571d0d9ddd3ad4f2aecbce907a9b7e3d0L124 to check if it looks good.

Bill-Gray commented 3 years ago

In re the order of file lookup : yes, that's basically what I had in mind. With the third search path being resettable, and possibly a multi-directory path ("look here, then here, then...")

For running multiple instances, use the -O (output directory) switch. ~/.find_orb will be used for input data files (observatory positions, observing weights, etc.), but output text, JSON, etc. files will go to the directory specified by -O. (Something CSS needed for NEOFixer, where they are indeed running a bazillion processes at once to evaluate visibility of a slew of objects multiplied by a slew of observing locations.)

Before writing the above paragraph, I did a few tests and found that some files were not properly redirected (didn't cause problems because they weren't files CSS was using). I've fixed the exceptions and pushed the changes.

I like both the fix for ncursesw detection to look for both 'find' and 'found', and the DEPENDABLE_VAR fix. The latter may have a variety of uses; it's not just PREFIX that can be defined on the make command line. CURSES_LIB can be reset if we're linking to PDCursesMod instead of ncurses. There's W64 and W32 for cross-compiling 64-bit and 32-bit Windows versions, and CFLAGS (which changes if DEBUG is turned on). We might easily end up with quite the series of 'dependable vars'.

We may be chasing down further ncursesw detection improvements, but this at least ought to be a step beyond just looking to see if we're on MacOS.

mjuric commented 3 years ago

@Bill-Gray Sounds good!

Good to know that the writes to ~/.find_orb with -O are a bug -- I assumed they were intentional (which is how I came up with the copying workaround).

Would a good way to proceed be to merge this PR (which would unblock some of the packaging work on conda-forge), but then quickly turn around and fix up the unintentional ~/.find_orb writing bugs (so the copying hack can be excised)?