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

single Makefile.inc and build.sh for scotch and ptscotch #15

Closed
wants to merge 6 commits into from

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 7, 2017

only include deviations from default in platform Makefiles

build number is bumped because -DSCOTCH_PTHREAD is restored, which had been removed.

only include deviations from default in platform Makefiles
because we restored -DSCOTCH_PTHREAD
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 7, 2017

Sorry about the removal of -DSCOTCH_PTHREAD . This bug slipped from my ongoing work on ptscotch. For the parallel variant, IMHO we should not use that flag. Too bad that scotch only allows compile-time configuration of threads.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 7, 2017

@minrk Either on Linux or macOS we use GNU Make. Then, why don't we just provide Makefile.inc and inside it do:

ifeq ($(shell uname ),Linux)
    ABC = xyz
else
    ABC = xyz
endif

RANLIB = ranlib
YACC = bison -pscotchyy -y -b y
CLIBFLAGS := $(CLIBFLAGS) -fPIC
LDFLAGS := $(LDFLAGS) -Wl,-headerpad_max_install_names
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we are doing something wrong here? Isn't conda build supposed to pass -Wl,-headerpad_max_install_names in the LDFLAGS environ var?

CCD = gcc
CFLAGS = -I${PREFIX}/include -O3 -DIDXSIZE64 -Drestrict=__restrict -DCOMMON_FILE_COMPRESS_GZ -DCOMMON_RANDOM_FIXED_SEED -DSCOTCH_RENAME -DCOMMON_PTHREAD -DCOMMON_PTHREAD_BARRIER -DSCOTCH_PTHREAD
CLIBFLAGS =
LDFLAGS = -L${PREFIX}/lib -lz -lm -pthread
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should do LDFLAGS := ${LDFLAGS} -L${PREFIX}/lib -lz -lm -pthread?

recipe/build.sh Outdated
cp $RECIPE_DIR/Makefile.inc.$(uname) src/Makefile.inc

cd src/
make esmumps | tee make.log 2>&1
make -j ${NUM_CPUS} esmumps | tee make.log 2>&1
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure all makefiles are properly written to support parallel builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just tested and they certainly aren't! Removed.

@dalcinl dalcinl mentioned this pull request Oct 8, 2017
@minrk minrk changed the title further simplify makefiles with Makefile.inc.common single Makefile.inc and build.sh for scotch and ptscotch Oct 9, 2017
AR = ar
ARFLAGS = -ruv
CAT = cat
CCS := ${CC}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you hae to do it this way:

CCS = ${CC}
CCP = ${CCS}
CCD = ${CCP}

Then, in build.sh, and only for ptscotch, export CCP=mpicc.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk I think you have parts of #16. The install step for ptscotch has to be different, otherwise you install both the parallel and sequential headers/libraries. That does not look right.

recipe/build.sh Outdated

if [[ "$(uname)" == "Darwin" ]]; then
export CFLAGS="${CFLAGS} -DCOMMON_PTHREAD_BARRIER -DCOMMON_TIMING_OLD"
export CLIBFLAGS="${CLIBFLAGS} -fPIC"
Copy link
Contributor

Choose a reason for hiding this comment

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

We need -fPIC both in Linux and macOS. Otherwise we may not be able to build a sharedlib PETSc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just seeting CLIBFLAGS = -fPIC in Makefile.inc is good enough?

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk Looking at your comment in #16, you are definitely right, ptscotch does not actually depend on scotch. The problem is that when you build ptscotch, you also get the scotch libraries and headers in the install tree. So I expect that some third-party libraries (that's the case of PETSc) would expect both the scotch and ptscotch variants at configure time. Making ptscotch depend on scotch would make things simpler. But of course we can just update the PETSc recipe to depend on both scotch and ptscotch.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

We also have a problem with the esmumps.h header file, there is no ptesmumps.h header, but we have libesmumps.a vs libptesmumps.a. Which package should own esmumps.h ?

@minrk
Copy link
Member Author

minrk commented Oct 9, 2017

Should the ptscotch install exclude the scotch libraries? I would think that installing ptscotch and scotch would be mutually exclusive, especially since the scotch build flags should be different depending on whether it's used for ptscotch or not (e.g. -DSCOTCH_PTHREAD for serial, but not for parallel)

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

Yes, for sure the ptscotch install exclude the scotch libraries. But our build.sh script was not doing that, we don't make install/ptinstall, just cp ... $PREFIX/<include,bin,lib> !. My changes in #16 fix that issue.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

Just to make things clear: Right now, the ptscotch package in anaconda.org contains sequential and parallel libraries, with the exceptions of libesmumps.a. That was the cause of the previous failure in conda-forge/petsc-feedstock#18 (now it is passing, after a quick test adding both scotch and ptscotch as dependencies).

However, installing both scotch and ptscotch together in a conda environment should work, all libraries have different names, the same for headers (with the exception of esmumps.h).

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk Oh! Hold on, ptscotch.h includes scotch.h !!

@minrk minrk closed this Oct 10, 2017
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.

None yet

3 participants