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

Build system improvements #322

Closed

Conversation

phil-blain
Copy link
Member

Improvements to the build system, add cice.make script

  • Developer(s): Philippe Blain

  • Please suggest code Pull Request reviewers in the column at right. @apcraig

  • Are the code changes bit for bit, different at roundoff level, or more substantial?
    BFB (only scripts are touched)

  • Please include the link to test results or paste the summary block from the bottom of the testing output below.
    -

  • Does this PR create or have dependencies on Icepack or any other models?
    No. I can issue a similar PR for Icepack if you agree with these changes.

  • Is the documentation being updated with this PR? (Y/N) Yes
    If not, does the documentation need to be updated separately at a later time? (Y/N)

Note: "Documentation" includes information on the wiki and .rst files in doc/source/,
which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/.

  • Other Relevant Details:
    • This PR adds a "cice.make" script that is added to the case directory (mostly a slimmed down "cice.build"). This script can be used to call make with the different goals that are defined in the Makefile, which are useful for debugging (db_files, db_flags, clean). I also updated the output of these flags.
    • I also changed the Makefile so that only a single "-I." flag is used for the call to the compiler, instead of having a "-Ipath/to/source/directory" flag for each source directory, which is useless in our case since all compiled modules are put in the ${ICE_OBJDIR} directory, where the compilation takes place (see commit message of 01f3873). I think this is a historical artifact dating from when the code used "include" instead of "use" directives.
    • Finally, I changed the Makefile so that the program 'makdep' is listed as a prerequisite of the rules that build the dependencies, so we don't need to call "make makdep" separately in cice.build to compile it.

* cice.make can be used to easily call make with the different targets
  present in the Makefile (db_files, db_flags, clean), or to pass
  different flags to make
* makdep is listed as a prerequisite of the *.d files, such that
  we don't need to call make makdep separately in cice.build
The -I flag is used by Fortran compilers to locate files
included via the "#include" preprocessor directive or the "include" statement,
or modules referenced via the "use" statement. In our codebase we only use
modules, and all modules files (*.mod) are put in the ${ICE_OBJDIR} directory,
where the compilation takes place. So there is no need for all source directories
(contained in the variable $(INCS), prefixed with -I) to be added to the build
rules. This shortens the length of the displayed compilation lines.
@apcraig
Copy link
Contributor

apcraig commented Jun 11, 2019

I think a bunch of these changes are good. I think the biggest question for me is the creation of the cice.make script. I think the main downsides is that it adds another script to the case directory (where I think less is generally better) and it seems to duplicate part of the cice.build script which creates extra maintenance issues. So, this leads to a few questions. Would it be possible for cice.build to be modified so it would support "cice.build target". In other words, could the cice.make features be built right into the cice.build script. Next, if we decide to keep the cice.make around, would it make more sense for the cice.build to call it, thereby reducing redundant code. Finally, if we feel giving users direct access to clean and other make targets is helpful, should we consider taking a bigger step which would be to have cice.build refactored, even getting rid of it all together and ending up with "make clean" or "make cice" as the way to build the model? I think my preference at this point is to see if cice.build can take an optional argument and get things working that way. Is that reasonable and do any of my comments make sense?

@phil-blain
Copy link
Member Author

Yes, what you say is reasonable and does make sense, thanks for your comments. I did not want to change things to much and that is why I created a new script. Here are my thoughts:

Would it be possible for cice.build to be modified so it would support "cice.build target". In other words, could the cice.make features be built right into the cice.build script.

I think it would be possible, but would complicate a little bit the logic of cice.build:

  1. the success/failure code after the call to make would only make sense if we compile everything, not for other make targets, so we would need additional logic
  2. right now the only argument accepted by cice.build is the path to an already built cice executable; we would need to add more checks to verify if the argument is a make goal or a path to an executable
  3. I guess we would only write the cice.bldlog file if we indeed compile the model, not call other Make goals, so additional logic would be needed here also

I think all of these are feasible.

Next, if we decide to keep the cice.make around, would it make more sense for the cice.build to call it, thereby reducing redundant code.

I think that is a good idea if we go that way.

should we consider taking a bigger step which would be to have cice.build refactored, even getting rid of it all together and ending up with "make clean" or "make cice" as the way to build the model?

From what I understand about how Make works with VPATH and all that I don't think it would work easily since the Makefile is in ICE_CASEDIR but the model is built in ICE_OBJDIR... and we would still need to call make with the parameters EXEC, VPFILE, MACFILE, as done in cice.build... I think having a wrapper script that is easy to call, as we have now, is a better way.

What do you think? I will update this PR depending on what we decide.

@apcraig
Copy link
Contributor

apcraig commented Jun 12, 2019

Thanks @phil-blain, several good points. I think my preference would be to try to modify cice.build if we can make that work well and clean. I propose the following interfaces

cice.build [--exe binary] [target]

Lets leave --exe as a hidden feature. It's really used just for testing at this point. target is optional and the script should do what it does now if left unset. Lets check that --exe and target are not set together.

Otherwise, if target is set, what we should do is just set the env as needed then execute the gmake command with the target. There should be no interaction with README.case or other fluff.

The targets are dependent what's in the Makefile, and that may not be easy to access. Is it possible to add a target in the makefile that indicates the possible targets? Like

cice.build targets

would return

cice.build support the following targets: cice, db_files, db_flags, clean, targets

I think we should do something with the "all" target in the Makefile. Can we add a "cice" target that is the same thing and make that the first target? Is "all" something that we really need/want in the Makefile to conform? I'm not a make expert.

Finally, I think if someone does

cice.build cice

it should build the model fine without going thru the extra fluff. While

cice.build

goes thru the extra fluff. Does that make sense? Thoughts?

@phil-blain
Copy link
Member Author

Yes, I think that this design would work well.

  • I can add a target to list the different targets.
  • The 'all' target is a make convention, if you type 'make' in a directory with a Makefile in it, make would execute the recipe for the first target in the Makefile, which by convention is 'all' and which builds everything. That is what we have now : cice.build internally calls make with different variables but without any goal, and thus make executes the recipe for the default target, which is 'all', which has EXEC as a prereq and thus builds the whole model. I think we should leave this as is.
  • it is a good idea to have a 'cice' target to just build the model without the logging fluff.
    I'll start working on that.

@apcraig
Copy link
Contributor

apcraig commented Jul 13, 2019

@phil-blain , sounds like your plate is full at the moment. Would you be OK if I took a crack at some of these changes if I get a chance? If so, I could branch off your branch and PR back to your branch keeping this PR alive. Does that sound reasonable? If you prefer to take this on yourself, that's OK too. Either way is fine.

@phil-blain
Copy link
Member Author

Yes I do have a lot of stuff at the moment. If you have a chance to work on this, go ahead.

@apcraig
Copy link
Contributor

apcraig commented Aug 15, 2019

Just FYI. I have been working on this the last couple days and should have a PR soon. I will include the #283 issue in the documentation.

@apcraig apcraig mentioned this pull request Aug 16, 2019
16 tasks
@apcraig
Copy link
Contributor

apcraig commented Aug 16, 2019

#354 supercedes this PR. #354 is the current master, with this PR merged in, and further changes. I will close this for now. I can be reopened if needed.

@apcraig apcraig closed this Aug 16, 2019
@phil-blain phil-blain deleted the build-system-improvements branch August 30, 2019 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants