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

[DO NOT MERGE] single build.sh + Makefile for scotch + ptscotch #17

Closed
wants to merge 6 commits into from

Conversation

minrk
Copy link
Member

@minrk minrk commented Oct 9, 2017

builds on #16

  • single build.sh for both scotch and ptscotch
  • move CFLAGS/etc. logic to build.sh, which is much simpler than conditionals in Makefile
  • remove unused pthread patch for case that's not relevant
  • run mpi tests on Darwin, working around mpich O_NONBLOCK error

This build.sh and Make.inc can be used exactly as-is on master

dalcinl and others added 6 commits October 8, 2017 11:18
- Single Makefile.inc (requires GNU Make).
- Do not define `SCOTCH_PTHREAD`. Number of threads is a compile-time
  setting and defaults to one. Client code is forced to call
  `MPI_Init_thread()` requesting `MPI_THREAD_MULTIPLE` support.
- Build/check/install/test scotch, then ptscotch. This should simplify
  a future split in two, with ptscotch depending on scotch and providing
  only the parallel headers and libraries.
- Add trivial patch for a missing function prototype.
- Add pthread patch from https://bitbucket.org/petsc/pkg-scotch.
- Bump build number.
include mpiexec.sh to fix O_NONBLOCK error
much simpler than in the Makefile
it only handles missing pthread, which doesn't occur
@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.

@minrk minrk changed the title single build.sh + Makefile for scotch + ptscotch [ptscotch] single build.sh + Makefile for scotch + ptscotch Oct 9, 2017
@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

Why you removed patch2? If we (or others) ever want to build without thread support, we need that patch. What's the harm of keeping it?

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk Could you please wait until I can finish to work in some other issues in #16, then you can take over and do whatever refactor (like moving logics from make to bash)?

I have the feeling that we are stepping over each other without proper discussion.

  • We have agreed that scotch should enable threads, and ptscotch should not. Good.
  • What about the provided headers and libraries? Should ptscotch provide sequential+parallel headers/librareis, or just the parallel ones?
  • Is your on-noblock trick is supposed to work both on Circle-CI and Travis-CI?

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk When you "make ptesmumps", you get almost all sequential+parallel headers and libraries, EXCEPT that the sequential libesmumps.a is not there, and then PETSc fails to configure.

@minrk
Copy link
Member Author

minrk commented Oct 9, 2017

Isn’t that a bug in petsc configuration if it fails to configure when serial scotch isn’t present, and it’s supposed to use ptscotch?

@minrk
Copy link
Member Author

minrk commented Oct 9, 2017

I’ll stop working on this until you are done. I removed the unused threads patch because it only applies to a case that isn’t relevant: pthreads being unavailable. That doesn’t occur for conda-forge, so I wanted to keep the deviations from upstream minimal.

I expected the noblock patch to work on circle, but it doesn’t appear to, so we are still stuck not testing on Linux.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

Perhaps the issue in PETSc with libesmumps.a is actually a bug, but remember that supports --download-xxx other packages that depend on SCOTCH/PTSCOTCH, so maybe PETSc is just being picky and checking for the full thing. I'll ask upstream.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk about the thread patch, I was thinking on users that for any reason may need to do a manual, local rebuild of this package with pthreads disabled. They could just grab our recipe and do minimal modifications to achieve that goal (in my original Makefile.inc, that can be done by just setting pthread = 0). Given that the patch does no harm to our build, and it might be useful for others, that's the reason I decided to grab it from PETSc's custom scotch repo.

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

@minrk I'm almost done with #16. Note the different build install path for ptscotch, now the ptscotch package only contains the parallel headers and libraries. So, in short, the files provided by scotch and ptscotch are disjoint sets.

The only issue that remains is the header epsmumps.h, right now the ptscotch package does not provide it, but it do provide libptesmumps.a. I'm not sure what to do about it. Maybe ship it anyway? What conda does if two packages provide the same file are are installed together?

@dalcinl
Copy link
Contributor

dalcinl commented Oct 9, 2017

About the configure logics in build.sh, maybe we could add all that in a separate config.sh script and then source $RECIPE_DIR/config.sh at the beginning of build.sh ?

@minrk
Copy link
Member Author

minrk commented Oct 10, 2017

Thanks! #18 applies this PR to master, plus moving the logic to build.sh. I didn't end up adding a config.sh since configuring the build is already what build.sh is for, and moving a couple of lines to a different script only to source it didn't make a lot of sense to me.

Maybe ship it anyway? What conda does if two packages provide the same file are are installed together?

conda doesn't check for conflicts, so I think whatever package happens to get installed last will own the file. If it's the same file, then it makes sense for it to be in scotch and for ptscotch to depend on scotch, right?

@minrk minrk changed the title [ptscotch] single build.sh + Makefile for scotch + ptscotch [DO NOT MERGE] single build.sh + Makefile for scotch + ptscotch Oct 10, 2017
@dalcinl dalcinl closed this Oct 11, 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