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 build script for developers, update Orion NCEPLIBS location #53

Conversation

mkavulich
Copy link
Collaborator

DESCRIPTION OF CHANGES:

Per discussion on #45, adding a build script for developer use. This will not be supported for community use, but limited to official platforms for testing purposes.

The build script should be invoked with the platform name and compiler as arguments, and works by sourcing the README files in the docs/ directory that describe the environment setup for each platform.

The README file for Orion also needed to be updated due to a change in the NCEPLIBS location to a more official spot.

TESTS CONDUCTED:

Built successfully on Cheyenne (gnu, intel) and Orion (intel). Should work for any platform with the appropriate README_[platform]_[compiler].txt file.

Copy link
Collaborator

@climbfuji climbfuji left a comment

Choose a reason for hiding this comment

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

I don't know why we need this extra build script on top of the ufs-weather-model build.sh script, but ok (I wasn't part of the discussion in #45). You are now sourcing README files with ending .txt as part of this build script. This is confusing. It's fine to do something like that, but we should rename those scripts from README_*.txt to SOMETHINGMEANINGFUL_*.sh in my opinion.

devbuild.sh Outdated

usage () {
echo "Usage: "
echo " ./build.sh PLATFORM COMPILER"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this message say "./devbuild.sh ..." instead of "./build.sh ..."?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I have updated the script to use the builtin $0 variable instead (script name)

@gsketefian
Copy link
Collaborator

I don't know why we need this extra build script on top of the ufs-weather-model build.sh script, but ok (I wasn't part of the discussion in #45). You are now sourcing README files with ending .txt as part of this build script. This is confusing. It's fine to do something like that, but we should rename those scripts from README_*.txt to SOMETHINGMEANINGFUL_*.sh in my opinion.

I think this is to build all the codes (not just ufs-weather-model) without having to cut-and-paste a bunch of stuff from README files. I'd agree that it would be clearer if the README_*.txt files were renamed to represent what they've really become (shell scripts), but I won't insist on it; I'll take what I can get!

@mkavulich
Copy link
Collaborator Author

mkavulich commented Nov 16, 2020

@climbfuji I know we have had this conversation several times now but we keep coming back to the same points. Some developers want a top-level build script so that we don't have to copy a dozen or so commands every time we build the full App, and I have trouble justifying the lack of one (aside from the desire to not support such a script to the community) so I am trying to reach a compromise. I will roughly copy my screed from the last PR:

I do not want to support a build script to the community that will almost definitely not work out of the box for most machines. But I understand what others have been saying about making more work for ourselves: for us testers (who will eventually be in the vast minority of all users) it is tremendously annoying to have to do several manual steps every time you build the model. Heck, it would be easier for me as well. A build script would also be desirable for automated testing.

So here is my final proposal: I will open a separate PR that includes a build script--the one that I developed earlier but never shared the details of. It will be quite simple and simply take a platform and (optional) compiler argument, and source the README files which will remain in valid bash format as they are now. In this way, we will not need to maintain two different systems, but merely a script on top of the official, documented build system. However, we will not include this build script in any public documentation (I'd prefer to not have it in the released code at all, but I don't think that's easy to do if we're releasing through github). So it will only be for use in testing and internal development. I'll tag the build team members that were especially vocal to make sure that we don't blindside anyone who might still have arguments against this idea.

Regarding the naming of the README files, I do not really have a strong opinion, but their main purpose is to be read and sourcing them is a secondary use. This script will only be used by a small number of internal developers. We could even remove it before the release if we are concerned about users trying to use it improperly.

@climbfuji
Copy link
Collaborator

@climbfuji I know we have had this conversation several times now but we keep coming back to the same points. Some developers want a top-level build script so that we don't have to copy a dozen or so commands every time we build the full App, and I have trouble justifying the lack of one (aside from the desire to not support such a script to the community) so I am trying to reach a compromise. I will roughly copy my screed from the last PR:

I do not want to support a build script to the community that will almost definitely not work out of the box for most machines. But I understand what others have been saying about making more work for ourselves: for us testers (who will eventually be in the vast minority of all users) it is tremendously annoying to have to do several manual steps every time you build the model. Heck, it would be easier for me as well. A build script would also be desirable for automated testing.

So here is my final proposal: I will open a separate PR that includes a build script--the one that I developed earlier but never shared the details of. It will be quite simple and simply take a platform and (optional) compiler argument, and source the README files which will remain in valid bash format as they are now. In this way, we will not need to maintain two different systems, but merely a script on top of the official, documented build system. However, we will not include this build script in any public documentation (I'd prefer to not have it in the released code at all, but I don't think that's easy to do if we're releasing through github). So it will only be for use in testing and internal development. I'll tag the build team members that were especially vocal to make sure that we don't blindside anyone who might still have arguments against this idea.

Regarding the naming of the README files, I do not really have a strong opinion, but their main purpose is to be read and sourcing them is a secondary use. This script will only be used by a small number of internal developers. We could even remove it before the release if we are concerned about users trying to use it improperly.

As I said, I don't have a problem with the build script being there, just wondering. But I bet that we'll get at least one user request about a failing build using it ;-) If your README files serve two purposes and are primarily used for copy & paste for the users of the SRW App release, then it's better to keep them as they are, agreed. If they are primarily used for the build script, then they should be renamed.

Copy link
Collaborator

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

LGTM.

@JeffBeck-NOAA JeffBeck-NOAA merged commit 407496b into ufs-community:release/public-v1 Dec 4, 2020
christinaholtNOAA pushed a commit to christinaholtNOAA/ufs-srweather-app that referenced this pull request Sep 7, 2021
@mkavulich mkavulich deleted the top-level-build-script branch September 8, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants