SKIRT / SKIRT9

SKIRT version 9 -- advanced radiative transfer in dusty systems
http://www.skirt.ugent.be
GNU Affero General Public License v3.0
37 stars 31 forks source link

Add GitHub release with auto download resources feature #169

Closed dirkarnez closed 1 year ago

dirkarnez commented 1 year ago

Description Describe the changes proposed in this pull request. Add GitHub release with auto download resources feature

Motivation Reference the issue(s) addressed by this pull request or provide other motivation. I tried to follow downloadResources.sh to download resources, but it took me quite a long time to figure out the correct resources

Tests How have you tested the changes proposed in this pull request? Yes, my notebook and GitHub action https://github.com/dirkarnez/SKIRT9/blob/master/.github/workflows/build.yml

Guidelines Does this pull request conform to the project's contributor guidelines? Yes

Context Add any other context about the pull request here. I cannot further test my pull request on OS other than Windows.

dirkarnez commented 1 year ago

From my fork's releases the SKIRT9 resources are contained, users do not need to download resources themselves

petercamps commented 1 year ago

Thank you for this pull request. Most users run SKIRT on Unix-flavored systems so we don't have much experience with running it on Windows. Your ideas and expertise are thus very welcome. May I ask, out of curiosity, what your interest is in the code?

To put things in context, I should mention that SKIRT is being used with many different operating systems and compilers, and SKIRT users often apply small changes or add minor features to their local copy of the code. It is therefore impractical to provide fixed releases or prebuilt binaries. In any case, it seems that your pull request addresses two distinct issues, and I have some questions about both.

GitHub workflow to check build on windows

This would be a nice addition to the check-builds workflow that's already there.

Automated download of resources

While the downloadResources shell script seems to work acceptably well for most Linux and macOS users, it does not work on Windows, forcing users to download and place the files manually. The relevant section in the installation guide has recently been somewhat improved, but an automated procedure would obviously be welcome.

Two requirements complicate such an automated procedure:

It thus seems best to keep the expected resource pack versions in a text file inside the repo so that the code can verify the installed versions as it does now. Perhaps the CMake download procedure can parse the text file and equip itself with build options for downloading each optional pack, so that these options can be configured on the CMake command line or other CMake user interfaces. And it would then also need to check for already installed packs.

Also, why are you copying the SKIRT executable (and all the resources) to the export subdirectory?

dirkarnez commented 1 year ago

@petercamps Thank you for the reply and interest in asking

I am just curious in astrophysics (not even a beginner) and discovered there are libraries like this exists.

check-builds

Automated download of resources

Copying the SKIRT executable (and all the resources) to the export subdirectory?

petercamps commented 1 year ago

@dirkarnez thank you for responses. (I now see that I accidentally overstated the size of the upcoming resource packs, but they will still be much larger than what we have now).

There won't be time on our side to address these issues in the very short term, but I will come back to them in a few weeks.

dirkarnez commented 1 year ago

@petercamps thanks

petercamps commented 1 year ago

Based on your suggestions, we added two jobs to the existing GitHub workflow for checking the SKIRT build at each pull request. These two new jobs use the Windows runner and compile and link SKIRT using MinGW-64 and MSVC, respectively. Testing with these extra compilers will certainly help ensure the cross-platform robustness of the code. See #171 for more information.

As for automatic resource downloading using CMake, the procedure would become rather involved. The procedure must pick the correct resource version, the user should be able to select which resources to download and which ones to skip, and the procedure should not do anything for resources that are already installed. The currently provided shell script works well on Unix platforms, and SKIRT is not used much on Windows. So in the end, we decided that it is not worth the effort to build a new procedure at this point in time.

Nevertheless, @dirkarnez, thank you for your ideas!

dirkarnez commented 1 year ago

@petercamps Thanks