Repository navigation
Update cfitsio to 3.41 - #6477
Conversation
mhvk
added
external
|
Hi there @mhvk 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labelled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 |
|
Hmm, appveyor fails with This file is not shipped with Hmm, seems it should have been excluded on windows (in |
|
cc @astrofrog, @eteq for the what-to-include packaging question Anybody who can help with the windows issue (linux seems fine; still waiting on MacOS) |
| # Define | ||
| ADD_DEFINITIONS(-D_CRT_SECURE_NO_DEPRECATE) | ||
| # Need an empty unistd.h in MSVC for flex-generated eval_l.c: | ||
| FILE(WRITE ${CMAKE_SOURCE_DIR}/unistd.h "") |
There was a problem hiding this comment.
Shouldn't take care of the missing unistd.h file?
There was a problem hiding this comment.
None of the Makefile get executed/included (certainly not now, I don't think before either - setup_package just includes *.c files), but obviously it should not be hard to mimic this in setup_package. I don't really understand, though, why this worked before...
Depends a bit on the built time. They probably won't hurt but if they significantly increase the build-time I'd say we should remove them again. |
|
The other files do not get built at all - everything relevant is moved to a new |
|
I don't feel strongly about this - @eteq @taldcroft? |
|
OK, now got the windows build working as well. I'm Cc'ing @embray as he's probably the last one to have done this and might have comments. Overall, it seems OK, but suggestions are still very welcome on the setup with |
I have no strong preference, on one hand staying close to the original package makes it easier to update, on the other hand we need only a subset of these files and some tweaks are needed anyway to compile cfitsio when included in Astropy (like moving files in a |
|
Okay, let's stick with the required files and remove all the (new) unused ones. Is that easily possible @mhvk? |
74e2a51 to
593c86f
Compare
|
@MSeifert04 - yes, not hard to remove all files, since I kept that as a single commit. Now removed them all, and adjusted the |
|
Thanks @mhvk , tests pass so I guess it's good to go 😅 |
MSeifert04
left a comment
MSeifert04
left a comment
There was a problem hiding this comment.
Wouldn't be a proper review if I can't find something to nitpick about :)
Otherwise 👍
| #!/bin/sh | ||
|
|
||
| # This script should be run every time cfitsio is updated. | ||
| # This moves allthe code needed for the actual library to lib |
There was a problem hiding this comment.
missing whitespace between all and the.
|
@MSeifert04 - OK, done as part of the rebase... |
|
Thanks @mhvk ! |
This updates
cfitsioto v3.41. One immediate question: I added a script that separates out the library files actually needed from sample programmes. Should we still include those additional files (alsoREADME, etc.)? I can easily not include them by removing the third commit, with possible adjustment oftrim_cfitsio.shto do this automatically.