MPAS-Dev / MPAS

Repository for private MPAS development prior to the MPAS v6.0 release.
Other
4 stars 0 forks source link

Added 'custom' target to build procedure. #1433

Closed cponder closed 6 years ago

cponder commented 7 years ago

Allow inheritance of build-parameters from calling environment.

This change adds a special "dummy" target where the build-parameters are expected to have been set externally:

  7 # Use settings from the invoking environment.
  8 custom:
  9         ( $(MAKE) all )

This change removes the distinction between "opt" and "debug" settings when the custom target is used:

498 elsifneq "$(MAKECMDGOALS)" "custom" # DEBUG IF
499         FFLAGS=$(FFLAGS_OPT)
500         CFLAGS=$(CFLAGS_OPT)
501         CXXFLAGS=$(CXXFLAGS_OPT)
502         LDFLAGS=$(LDFLAGS_OPT)
503         DEBUG_MESSAGE="Debugging is off."
504 endif # DEBUG IF
mgduda commented 7 years ago

@cponder I've added the Framework label, and we can discuss this in the next MPAS Developers' telecon on Monday.

An alternative might just be to use conditional assignment for all of the variables in the recursive make commands, e.g.,

gfortran:
        ( $(MAKE) all \
        "FC_PARALLEL ?= mpif90" \
        "CC_PARALLEL ?= mpicc" \
        "CXX_PARALLEL ?= mpicxx" \
        "FC_SERIAL ?= gfortran" \
        "CC_SERIAL ?= gcc" \
        "CXX_SERIAL ?= g++" \
        "FFLAGS_PROMOTION ?= -fdefault-real-8 -fdefault-double-8" \
        "FFLAGS_OPT ?= -O3 -m64 -ffree-line-length-none -fconvert=big-endian -ffree-form" \
        "CFLAGS_OPT ?= -O3 -m64" \
        "CXXFLAGS_OPT ?= -O3 -m64" \
        "LDFLAGS_OPT ?= -O3 -m64" \
        "FFLAGS_DEBUG ?= -g -m64 -ffree-line-length-none -fconvert=big-endian -ffree-form -fbounds-check -fbacktrace -ffpe-trap=invalid,zero,overflow" \
        "CFLAGS_DEBUG ?= -g -m64" \
        "CXXFLAGS_DEBUG ?= -O3 -m64" \
        "LDFLAGS_DEBUG ?= -g -m64" \
        "FFLAGS_OMP ?= -fopenmp" \
        "CFLAGS_OMP ?= -fopenmp" \
mgduda commented 7 years ago

@cponder Assuming we merge this PR after some discussion, could you also rewrite the commit message and first comment of this PR (which will be used for the merge commit message) in the style of the other commits/PRs? For the commit, the first line (<80 chars if at all possible) should be a one-line summary of the changes, then a blank line, then a description of the changes, their impact, etc., ideally with no first-person pronouns. For a one-commit PR, the merge commit message (i.e., the first comment of the PR) can largely reflect the commit message.

mgduda commented 7 years ago

@cponder To answer the question at the end of your comment, a "pull request" requests that the maintainers of a repository pull your changes into a particular branch; this allows the changes to be reviewed first, whereas allowing anyone to push changes directly would make that more difficult.

cponder commented 7 years ago

Michael -- point me to an example checkin-comment and I'll rewrite the one I have here.

mgduda commented 7 years ago

@cponder Here's an example of a nice, thorough commit message: https://github.com/MPAS-Dev/MPAS/commit/df2f3f2 .

mgduda commented 7 years ago

@cponder Here's another example: https://github.com/MPAS-Dev/MPAS/commit/073f61a .

cponder commented 7 years ago

cponder https://github.com/cponder commented 6 days ago https://github.com/MPAS-Dev/MPAS/pull/1433#issue-266167450

I'd like this change pushed into the main trunk for MPAS-A &
MPAS-O as well.
(I'm following Supreeth's instructions here. I'm still hazy on why
I'm using a "pull" request to push changes into the main repo, for
example).

On 10/18/2017 03:56 PM, Michael Duda wrote:

@cponder <https://github.com/cponder> To answer the question at
the end of your comment, a "pull request" requests that the
maintainers of a repository pull your changes into a particular
branch; this allows the changes to be reviewed first, whereas
allowing anyone to push changes directly would make that more
difficult.

Ok I think I'm starting to understand it -- there isn't a hierarchy of repo's where things get "pushed" upward, right? It's more like a peer-to-peer where you can "pull" things across from one repo to another. I'm too used to revision-control systems that aren't so flexible.

        -- Carl


This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message.

cponder commented 7 years ago

This ?= approach will work for my purposes. The side-effect is that all the targets behave identically when the variables have all been pre-set

          make pgi         ....
          make gfortran ....
          <etc.>

and this may be confusing to the reader. Also, with this particular change, the CFLAGS_OPT and FFLAGS_OPT would have to be used, since they are the default variables when DEBUG is not set, rather, than just setting CFLAGS and FFLAGS. The factorization of OPT versus DEBUG targets is convenient but makes less sense when a Score-P binary is being built, for example.

cponder commented 7 years ago

I've re-phrased the checkin comment to focus on "what" the change does, rather than "why" I recommend the change. For the record here, my objective is to have a "thin layer" script that controls the target being built, by setting up the environment paths and specifying the compilation parameters, without my having to modify the Makefile or any of the other source files. Then I'd have a script to build an optimized binary, a debug binary, a Score-P binary, etc., and each scripts would rename its binary appropriately. Ideally, if the MPAS-A source-base changes, the Makefile variables would stay the same even if other contents of the Makefile get changed, so these scripts wouldn't have to change. If I have to modify the contents of the Makefile, I expect that I could have to re-work it every time I get a new MPAS-A snapshot.

mgduda commented 7 years ago

@cponder There are just a few more things that need to be addressed before merging this PR:

mgduda commented 6 years ago

@cponder I'm going to close this PR for now. But, if you'd like to pick it up again, feel free to re-open it.

cponder commented 6 years ago

Did you merge this change into the trunk?

cponder commented 6 years ago

This message

>make custom CORE=atmosphere
Makefile:9: *** missing separator.  Stop.

likely means that the initial tab on line 9 had been converted to spaces, so the rule would not be correctly formed.