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

Add GNU Astro #9723

Merged
merged 18 commits into from
Oct 14, 2019
Merged

Add GNU Astro #9723

merged 18 commits into from
Oct 14, 2019

Conversation

sebastian-luna-valero
Copy link
Member

Checklist

  • License file is packaged (see here for an example)
  • Source is from official source
  • Package does not vend other packages
  • Build number is 0
  • A tarball (url) rather than a repo (e.g. git_url) is used in your
    recipe (see
    here
    for more details)
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/gnuastro) and found some lint.

Here's what I've got...

For recipes/gnuastro:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/gnuastro) and found it was in an excellent condition.

@mohammad-akhlaghi
Copy link

mohammad-akhlaghi commented Oct 4, 2019

Thanks a lot for initiating the Conda recipe for Gnuastro. Just a few comments on meta.yml:

  1. make check already does check the execution of all the programs, so maybe the --help tests are redundant and removing them can help in readability/maintaining meta.yml.

  2. Since I don't use Conda myself, I don't exactly understand the difference between the "host", "build" and "run" requirements, but if the GNU Libtool executable program (libtool or glibtool) is present in Gnuastro's ./configure step and later when the user runs the installed programs, another useful component of Gnuastro will be available: the program called BuildProgram.

  3. libgit2 is missing as a dependency. Of course, its optional, so Gnuastro's build won't fail without it. But not having disables a very important feature for reproducibility in particular. See the descriptions under COMMIT in this part of the Gnuastro book.

For the macOS builds, I tried pressing "details" in the summary above, but it just says "Bash exited with code '1'." If there is a more detailed error message of what caused the crash, I'd be happy to fix it upstream.

@sebastian-luna-valero
Copy link
Member Author

Thanks, @mohammad-akhlaghi

  • When I used make check, I got the following error at compilation time:
FAIL: buildprog/simpleio.sh
===========================

libtool: link: gcc -Wall -O3 -I../lib -I../lib/ -I/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/include ../tests/buildprog/simpleio.c -I/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/include -o .libs/simpleio   -L/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib ../lib/.libs/libgnuastro.so -ltiff -llzma -ljpeg -lwcs -lcfitsio -lcurl -lz -lgsl -lgslcblas -lm -lc -pthread -Wl,-rpath -Wl,/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib
/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/_build_env/bin/libtool: line 1734: gcc: command not found

Compiling and linking the program
---------------------------------
/home/conda/feedstock_root/build_artifacts/gnuastro_1570118628901/work/bin/buildprog/.libs/lt-astbuildprog: failed to build, see libtool error above
FAIL buildprog/simpleio.sh (exit status: 1)

However, when I commented make check, the other tests passed.

For the OSX builds, please have a look at:
https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_build/results?buildId=77641&view=logs&jobId=c572e5d5-7ba7-55ce-5575-7ce1d9f7161e

This time, please click on "Bash exited with code '1'." to see the full error log.

@mohammad-akhlaghi
Copy link

mohammad-akhlaghi commented Oct 5, 2019

Great! Thanks for the clarifications.

  • The build failure with make check is because it can't find a C compiler (gcc: command not found). You see, Gnuastro's BuildProgram is a actually a meta-program: a program to help users build small programs with Gnuastro's library. So it needs to have a C compiler available (gcc in this case) at run-time, not just build-time. So the solution is to add gcc as a dependency of the environment that runs make check (or the generic term Conda uses for the C compiler that is usable in macOS is well). Probably its under the Test requirements section of that page you sent.

  • Thanks again for the link on the various environments. Then I guess you have to add libtool (if this means GNU Libtool) to the "run" dependencies is well.

  • Unfortunately because I don't have a Conda environment installed or even setup right now, I would rather leave the commits to you ;-). You can test the commits and check them before pushing. I am too busy now to install Conda, but I will hopefully do it later when I get some free time.

  • The macOS problem was very interesting! I just defined bug #56999, so we also have it documented in Gnuastro. For now, how critical is this for the Conda packaging? I will try to look into it as soon as possible. I don't use macOS myself, but on every release, several volunteers do check Gnuastro on macOS and none had reported this problem before.

@sebastian-luna-valero
Copy link
Member Author

Hi,

No worries, I will keep pushing changes and we'll ask a conda-forge team to review the final recipe in due course.

While I work on the other changes, can I ask a more specific question about the make check issue? I think the issue here is that there is an explicit call to gcc rather than using $GCC to find the C compiler. If you let me know where the gcc call is performed, I will look into a solution. So far I had a look at tests/buildprog/simpleio.sh with no luck.

Best regards,
Sebastian

@mohammad-akhlaghi
Copy link

mohammad-akhlaghi commented Oct 7, 2019

Good point! Until now, gcc was actually hard-coded into the source of BuildProgram. I just implemented the solution upstream: it now has a special option, to let you choose the C compiler, or it will use the CC or GCC environment variables.

If you can use the most recent version (which is available here until the next stable release: https://akhlaghi.org/src/gnuastro-0.10.32-3fa6.tar.lz ), then the problem will be fixed. Otherwise, you can simply disable BuildProgram in Gnuastro with the --disable-buildprog configure-time option until the next stable release. When disabled at configure-time, BuildProgram won't be built and make check also won't attempt to check it, so it should finish without crashing anywhere.

@sebastian-luna-valero
Copy link
Member Author

Many thanks for the acknowledgement upstream!

Provided that http://ftp.gnu.org will be the default repository for new releases, I would like to stick to that in the recipe for this version of the package, and I will update it for the next one.

You are right, as soon as I added --disable-buildprog the make check step worked!

By the way, would you like to be listed as a recipe maintainer for the gnuastro package?

@sebastian-luna-valero
Copy link
Member Author

@conda-forge/help-c-cpp please review

@mohammad-akhlaghi
Copy link

Great! I also had the same idea about keeping with the stable releases.

To stay up to date, you can subscribe to the Gnuastro's announcements mailing list. Traffic on this list is very low by design: once or twice a season ;-).

About being listed as a maintainer, yes, no problem, just put me as second maintainer and add a note that I am the upstream developer ;-). I just hope that you'll be around to fix any conda-related problem that comes up, I'll be happy to fix the Gnuastro-releated problems.

@sebastian-luna-valero
Copy link
Member Author

Done.

Sure, I will be happy to help maintaining this recipe ;)

@sebastian-luna-valero
Copy link
Member Author

@conda-forge/staged-recipes please review


build:
number: 0
skip: True # [win or osx]
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 the macOS error is due to AT_FDCWD being added in OSX 10.10 (by default conda-forge targets 10.9). It might be easy to patch in the source? If not can add a file like this to target 10.10.

Suggested change
skip: True # [win or osx]
skip: True # [win]

Copy link
Member

Choose a reason for hiding this comment

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

I would change the target to 10.10 as a last resort and just patch the source.

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 think neither of us have access and/or experience with OSX, that's why we left it out.

@mohammad-akhlaghi would you be able to patch the source accordingly?

test:
commands:
- test -f ${PREFIX}/lib/libgnuastro${SHLIB_EXT} # [unix]
- test -f ${PREFIX}/lib/libgnuastro.a # [unix]
Copy link
Member

Choose a reason for hiding this comment

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

Are there any command line apps you can test? Even if it's just a simple program_name --help

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I had it included in 16656e2 but @mohammad-akhlaghi suggested me to remove those and use make check instead, since the CLI is tested according to the result of the ./configure

Copy link
Member

Choose a reason for hiding this comment

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

It's nice to check a second time as it tests that the relocation as worked as these tests are ran in a separate environment to the build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will do.


about:
home: https://www.gnu.org/software/gnuastro
license: GPL-3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
license: GPL-3
license: GPL-3.0-or-later


source:
url: http://ftp.gnu.org/gnu/{{name}}/{{name}}-{{version}}.tar.gz
sha256: 6d56dc161ecd80c64dbf09808f58114b52ad2ce648c71551b871f5d10d4c07c5
Copy link
Member

Choose a reason for hiding this comment

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

You could download http://git.savannah.gnu.org/cgit/gnuastro.git/patch/?id=3fa6512537ea1905583cf2b7dff45d174260c4c3 and apply it as a patch instead of passing --disable-buildprog:

Suggested change
sha256: 6d56dc161ecd80c64dbf09808f58114b52ad2ce648c71551b871f5d10d4c07c5
sha256: 6d56dc161ecd80c64dbf09808f58114b52ad2ce648c71551b871f5d10d4c07c5
patches:
- remove-hardcoded-gcc.patch

@mohammad-akhlaghi
Copy link

I tried creating the patch, but there were some complications when trying to apply it to the tarball of Gnuastro 0.10: already more than 30 commits have been made to Gnuastro since the 0.10 release, especially in the part that this commit is applied to the book, the line numbers don't match. Ofcourse, we can technically remove those parts from the patch, but then we'll have a feature that is not documented in Conda! So, I think that until Gnuastro 0.11 is released, --disable-buildprog is a good solution for now. It does disable one complete program, but atleast it is clearly documented for anyone who looks into the Conda build recipe.

@sebastian-luna-valero
Copy link
Member Author

Many thanks for your feedback.

I applied your patch and removed --disable-buildprog here and it works using the Gnuastro-0.10 tarball. However, if you think it is better to revert these changes to be in sync with the docs, I am happy to do it and we'll revisit the recipe for Gnuastro-0.11.

Sorry, could you please confirm what's your opinion on patching Gnuastro-0.10 to get OSX to work? (please see the comment about AT_FDCWD above)?

Best regards,
Sebastian

@mohammad-akhlaghi
Copy link

About the patch, either option is good ;-)! In any case, some change needs to be made compared to the default Gnuastro 0.10, but now that you have implemented the patch, we can just keep it. The good thing now is that Conda users will have access to astbuildprog. Thanks for doing this.

About the macOS problem with AT_FDCWD, maybe you can apply the solution suggested by @chrisburr, and set Condor to use a more recent macOS version like this recipe. I have added it as a bug in Gnuastro, and will try to find a fix as soon as I have some free time. Since it can be fixed with a newer macOS for Conda right now, I don't think its urgent (and I am really busy these days with a few deadline approaching fast!).

@sebastian-luna-valero
Copy link
Member Author

@chrisburr @isuruf should we target OSX 10.10 on this PR or should we wait for the next release of Gnuastro to include OSX?

Linux is key for us. OSX not high priority though.

@chrisburr
Copy link
Member

My preference would be to add OSX 10.10+ support now as I doubt there are many people left running 10.9 but @isuruf knows better than I do.

@mohammad-akhlaghi
Copy link

I had a fast look at the cases that AT_FDCWD comes up, and I think its easy to implement them in a lower-level way to avoid the need for it (to see if the user has write-permissions in a directory). The patch should be ready soon ;-).

@mohammad-akhlaghi
Copy link

I just removed the use of these macros. The problem on macOS should be fixed after applying this commit. Just remove the change in the NEWS file when making the patch (because it has changed since the 0.10 release). If this fixes the problem on macOS 10.9, then I'll close the Gnuastro bug also.

@sebastian-luna-valero
Copy link
Member Author

Ok, so I have now applied the patch but make check now fails on OSX with FAIL: buildprog/simpleio.sh

I troubleshooted this same error on Linux by running the tests locally, exec'ing into the container and inspecting the output of tests/test-suite.log. As I mentioned, I have no access to OSX locally, would any of you be able to do it?

@chrisburr
Copy link
Member

If it's still not obvious from the log I can take a look

@sebastian-luna-valero
Copy link
Member Author

That's a great addition to the recipe, many thanks @chrisburr

Do you understand the libgit2 issue?

@sebastian-luna-valero
Copy link
Member Author

@chrisburr @isuruf we would like to have this package added to conda-forge for a project we are currently doing, could you please let us know how to move on?

@mohammad-akhlaghi
Copy link

Thanks a lot @chrisburr and @isuruf. The problem seems to be in the @rpath that is built into the @rpath/libgit2.28.dylib of the test built program. When I compare with the build-instructions of the other programs (even the test programs compiled during make check), I see the term -Wl,-pie -Wl,-headerpad_max_install_names -Wl,-dead_strip_dylibs -Wl,-rpath,$PREFIX/lib which is not used in the compile command of Gnuastro's BuildProgram during that failed make check step.

I noticed that this is because until now, BuildProgram was ignoring the LDFLAGS environment variable in its build instruction. So I just committed the fix to Gnuastro's source. As described in the commit message, this should fix this problem.

Can you please use this commit to patch the Gnuastro build (the patch should be applied after the previous GCC-related patch). Also, don't forget to remove the changes in the NEWS and documentation (doc/gnuastro.texi) files.

@chrisburr
Copy link
Member

I think it's there!

Thank you @sebastian-luna-valero and @mohammad-akhlaghi for taking the time to package this "correctly", LGTM 🍰

@chrisburr chrisburr merged commit 330dcd3 into conda-forge:master Oct 14, 2019
@sebastian-luna-valero
Copy link
Member Author

Thanks All.

FYI: I have intentionally removed the changes in NEWS and doc/gnuastro.texi since the patching step in conda failed. I am not sure why, I am afraid.

Can we revisit this for the next gnuastro release?

@mohammad-akhlaghi
Copy link

Indeed, thanks a lot everyone :-D.

With two bugs found and some new features added, the Conda packaging was also very productive upstream: I really like how every package manager has different properties that helps highlight different bugs/needs, thus making the software more robust.

When Gnuastro 0.11 comes, none of these patches will be necessary any more.

Sebastian, the reason the patch failed on these two files is that many different commits (outside the Conda-related commits) have been made on these two files since Gnuastro 0.10 was released: the NEWS file reports new features in every release and the doc/gnuastro.texi is the source of Gnuastro's documentation. These two files usually get updated with almost any commit! So the line numbers of the Conda-commits don't match with the 0.10 source.

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

Successfully merging this pull request may close these issues.

5 participants