-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[workspace] Upgrade petsc to latest release v3.17.1 #17119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@xuchenhan-tri for feature review on the patch changes please
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @xuchenhan-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something went sideways, removing reviewer for now -@xuchenhan-tri
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes
00068bb
to
238dc41
Compare
This is... odd? I would not expect the patch to need to gain an additional line of context. |
7e3f7fe
to
545d866
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @BetsyMcPhail and @mwoehlke-kitware)
tools/workspace/petsc/stubs/petscconf.h
line 120 at r3 (raw file):
#define PETSC_HAVE_SNPRINTF 1 #define PETSC_HAVE_SOCKET 1 #define PETSC_HAVE_SOWING 1
Ack, this is my bad 😳. This line (#define PETSC_HAVE_SOWING 1
) probably should not be added.
5e7fa90
to
5b46661
Compare
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
Nit (my bad!): Suggestion: --with-sowing=0 |
5b46661
to
bf2861a
Compare
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
bf2861a
to
84cf71a
Compare
@drake-jenkins-bot mac-big-sur-clang-bazel-experimental-release please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@xuchenhan-tri for review please. Although the patch files look like a lot of changes, it is due to renaming within the Petsc code itself. The patch logic remained the same and applied easily.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @mwoehlke-kitware and @xuchenhan-tri)
tools/workspace/petsc/stubs/petscconf.h
line 120 at r3 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Ack, this is my bad 😳. This line (
#define PETSC_HAVE_SOWING 1
) probably should not be added.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@jwnimmer-tri for review as well.
The patch rebasing is the easy part, it's the large number of petscconf.h changes are the more important part of this PR.
Across the version number change, if upstream added new macros to is configure script then it's appropriate to add definitions for those here, or if upstream dropped macros from its configure script (perhaps PETSC_HAVE_CXX_DIALECT_CXX03
?) then it's appropriate to remove those here. Otherwise, there should be no other changes to petscconf.h, yet I see a ton of what appear to be changes to petscconf.h, unrelated to a version upgrade. At a minimum, those changes need some explanation in the PR overview, but I suspect they are actually just unwanted.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @jwnimmer-tri, @mwoehlke-kitware, and @xuchenhan-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My plan is to re-do the petscconf.h changes myself and push a fixup here. If you have specific information about added/removed config switches from your exploration, let me know.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @jwnimmer-tri, @mwoehlke-kitware, and @xuchenhan-tri)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @mwoehlke-kitware from a discussion.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @jwnimmer-tri and @xuchenhan-tri)
tools/workspace/petsc/stubs/petscconf.h
line 113 at r7 (raw file):
#define PETSC_HAVE_SLEEP 1 #define PETSC_HAVE_SNPRINTF 1 #define PETSC_HAVE_SOCKET 1
Working
We do not let petsc talk to the network.
tools/workspace/petsc/stubs/petscconf.h
line 185 at r7 (raw file):
#define PETSC_UINTPTR_T uintptr_t #define PETSC_UNUSED __attribute((unused)) #define PETSC_USE_AVX512_KERNELS 1
Working
Drake must work on non-AVX512 machines.
tools/workspace/petsc/stubs/petscconf.h
line 201 at r7 (raw file):
#define PETSC_USING_DARWIN 1 #endif #define PETSC_USING_F2003 1
Working
We don't use the Fortran build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mwoehlke-kitware can you offer any insight into the changes in petscconf.h?
Reviewable status: 3 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @jwnimmer-tri and @xuchenhan-tri)
When was the last time anyone recreated this? I just ran |
It's true that Xuchen and I need to improve the documentation and process around this file. We have some assumptions in play that were not written down. We'll work on improving that, as we take over this PR now. It was not my intention for anyone else other than him and myself to try to edit this file without our guidance. I'll be sure to document that more clearly as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @BetsyMcPhail, @jwnimmer-tri, and @xuchenhan-tri)
tools/workspace/petsc/stubs/petscconf.h
line 58 at r7 (raw file):
#define PETSC_HAVE_FLOAT_H 1 #define PETSC_HAVE_FORK 1 #define PETSC_HAVE_FSTATAT 1
In general, why would we not use features available on the platform? I'll comment on specific exceptions, otherwise I think any additions of this nature should be kept unless there is specific justification otherwise.
tools/workspace/petsc/stubs/petscconf.h
line 76 at r7 (raw file):
#define PETSC_HAVE_MMAP 1 #define PETSC_HAVE_MPIUNI 1 #define PETSC_HAVE_MPI_IN_PLACE 1
These three no longer appear in the PETSc code.
tools/workspace/petsc/stubs/petscconf.h
line 100 at r7 (raw file):
#define PETSC_HAVE_REALPATH 1 #ifndef __APPLE__ #define PETSC_HAVE_REAL___FLOAT128 1
This was not defined on x86 macOS. Possibly PETSc knows not to use it anyway, but explicitly removing it from macOS seems safer.
tools/workspace/petsc/stubs/petscconf.h
line 113 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
We do not let petsc talk to the network.
Okay. We might want to consider commenting these out rather than simply removing them, with a note, so that next time someone wants to update this, the knowledge is not lost.
tools/workspace/petsc/stubs/petscconf.h
line 129 at r7 (raw file):
#define PETSC_HAVE_SYS_SOCKET_H 1 #ifdef __APPLE__ #define PETSC_HAVE_SYS_SYSCTL_H 1
It appears that this (and PETSC_RTLD_DEFAULT
) are defined but never used. My inclination would be to keep them to minimize unnecessary differences vs. the generated version.
tools/workspace/petsc/stubs/petscconf.h
line 161 at r7 (raw file):
#define PETSC_MAX_PATH_LEN 4096 #endif #define PETSC_MEMALIGN 16
This should be mostly harmless, at worst, and very possibly a performance improvement.
I have the following (below) "latest" version, which should be "exactly" what comes out of
I think we should retain these changes; it brings us closer to upstream and makes it clear where we are making additional, non-obvious changes so that these can be retained. This should make it much easier to update the #pragma once
// TODO(jwnimmer-tri): This file is a lightly-edited version of what comes out
// of upstream's configure script that has been tweaked to work on all of our
// supported platforms, and to remove some unwanted bits. Commented out
// definitions indicate things present in the generated version which we have
// deliberately removed.
//
// This file can be (approximately) recreated using:
// ./configure --with-log=0 --with-x=0 --with-mpi=0 --with-sowing 0 \
// --with-fortran-bindings=0 --with-matlab-socket=0 \
// --with-debugging=0
#define MPI_Comm_create_errhandler(p_err_fun, p_errhandler) \
MPI_Errhandler_create((p_err_fun), (p_errhandler))
#define MPI_Comm_set_errhandler(comm, p_errhandler) \
MPI_Errhandler_set((comm), (p_errhandler))
#define MPI_Type_create_struct(count, lens, displs, types, newtype) \
MPI_Type_struct((count), (lens), (displs), (types), (newtype))
#define PETSC_ARCH "NO_PETSC_ARCH"
#define PETSC_ATTRIBUTEALIGNED(size) __attribute((aligned(size)))
#define PETSC_Alignx(a, b)
#ifdef __APPLE__
# define PETSC_BLASLAPACK_SDOT_RETURNS_DOUBLE 1
# define PETSC_BLASLAPACK_SNRM2_RETURNS_DOUBLE 1
#endif
#define PETSC_BLASLAPACK_UNDERSCORE 1
#define PETSC_CLANGUAGE_C 1
#define PETSC_CXX_RESTRICT __restrict
#define PETSC_DEPRECATED_ENUM(why) __attribute((deprecated))
#define PETSC_DEPRECATED_FUNCTION(why) __attribute((deprecated))
#define PETSC_DEPRECATED_MACRO(why) _Pragma(why)
#define PETSC_DEPRECATED_TYPEDEF(why) __attribute((deprecated))
#define PETSC_DIR "NO_PETSC_DIR"
#define PETSC_DIR_SEPARATOR '/'
#ifdef __APPLE__
# define PETSC_DO_NOT_SWAP_CHILD_FOR_DEBUGGER 1
#endif
//#define PETSC_FORTRAN_* - Not using Fortran, shouldn't need these.
#define PETSC_FUNCTION_NAME_C __func__
#define PETSC_FUNCTION_NAME_CXX __func__
#define PETSC_HAVE_ACCESS 1
#define PETSC_HAVE_ATOLL 1
#define PETSC_HAVE_ATTRIBUTEALIGNED 1
#define PETSC_HAVE_BUILTIN_EXPECT 1
#define PETSC_HAVE_BZERO 1
#define PETSC_HAVE_C99_COMPLEX 1
#define PETSC_HAVE_CLOCK 1
#ifdef __APPLE__
# define PETSC_HAVE_CLOSURE 1
#endif
#define PETSC_HAVE_CXX 1
#ifdef __APPLE__
# define PETSC_HAVE_CXXABI_H 1
#endif
#define PETSC_HAVE_CXX_COMPLEX 1
#define PETSC_HAVE_CXX_COMPLEX_FIX 1
#define PETSC_HAVE_CXX_DIALECT_CXX11 1
#define PETSC_HAVE_CXX_DIALECT_CXX14 1
#define PETSC_HAVE_CXX_DIALECT_CXX17 1
#define PETSC_HAVE_DLADDR 1
#define PETSC_HAVE_DLCLOSE 1
#define PETSC_HAVE_DLERROR 1
#define PETSC_HAVE_DLFCN_H 1
#define PETSC_HAVE_DLOPEN 1
#define PETSC_HAVE_DLSYM 1
#define PETSC_HAVE_DOUBLE_ALIGN_MALLOC 1
#define PETSC_HAVE_DRAND48 1
#define PETSC_HAVE_DYNAMIC_LIBRARIES 1
#define PETSC_HAVE_ERF 1
#define PETSC_HAVE_FCNTL_H 1
#define PETSC_HAVE_FENV_H 1
#define PETSC_HAVE_FLOAT_H 1
#define PETSC_HAVE_FORK 1
//#define PETSC_HAVE_FORTRAN_* - Not using Fortran, shouldn't need these.
#define PETSC_HAVE_GETCWD 1
#define PETSC_HAVE_GETDOMAINNAME 1
#define PETSC_HAVE_GETHOSTBYNAME 1
#define PETSC_HAVE_GETHOSTNAME 1
#define PETSC_HAVE_GETPAGESIZE 1
#define PETSC_HAVE_GETRUSAGE 1
#ifdef __APPLE__
# define PETSC_HAVE_GETWD 1
#endif
#define PETSC_HAVE_IMMINTRIN_H 1
#define PETSC_HAVE_INTTYPES_H 1
#define PETSC_HAVE_ISINF 1
#define PETSC_HAVE_ISNAN 1
#define PETSC_HAVE_ISNORMAL 1
#define PETSC_HAVE_LGAMMA 1
#define PETSC_HAVE_LOG2 1
#define PETSC_HAVE_LSEEK 1
#ifdef __APPLE__
# define PETSC_HAVE_MACHINE_ENDIAN_H 1
#else
# define PETSC_HAVE_MALLOC_H 1
# define PETSC_HAVE_MEMALIGN 1
#endif
#define PETSC_HAVE_MEMMOVE 1
#define PETSC_HAVE_MMAP 1
#define PETSC_HAVE_MPIUNI 1
#define PETSC_HAVE_NANOSLEEP 1
#define PETSC_HAVE_NETDB_H 1
#define PETSC_HAVE_NETINET_IN_H 1
#define PETSC_HAVE_PACKAGES ":blaslapack:mathlib:mpi:pthread:regex:"
#define PETSC_HAVE_POPEN 1
#define PETSC_HAVE_PTHREAD 1
#ifndef __APPLE__
# define PETSC_HAVE_PTHREAD_BARRIER_T 1
#endif
#define PETSC_HAVE_PTHREAD_H 1
#define PETSC_HAVE_PWD_H 1
#define PETSC_HAVE_RAND 1
#define PETSC_HAVE_READLINK 1
#define PETSC_HAVE_REALPATH 1
#ifndef __APPLE__
# define PETSC_HAVE_REALFLOAT128 1
#endif
#define PETSC_HAVE_REGEX 1
#define PETSC_HAVE_RTLD_GLOBAL 1
#define PETSC_HAVE_RTLD_LAZY 1
#define PETSC_HAVE_RTLD_LOCAL 1
#define PETSC_HAVE_RTLD_NOW 1
#ifndef __APPLE__
# define PETSC_HAVE_SCHED_CPU_SET_T 1
#endif
#define PETSC_HAVE_SETJMP_H 1
#define PETSC_HAVE_SLEEP 1
#define PETSC_HAVE_SNPRINTF 1
//#define PETSC_HAVE_SOCKET 1 - Don't allow network access.
//#define PETSC_HAVE_SO_REUSEADDR 1 - Don't allow network access.
#define PETSC_HAVE_STDINT_H 1
#define PETSC_HAVE_STRCASECMP 1
#define PETSC_HAVE_STRINGS_H 1
#define PETSC_HAVE_STRUCT_SIGACTION 1
#ifndef __APPLE__
# define PETSC_HAVE_SYSINFO 1
#endif
#define PETSC_HAVE_SYS_PARAM_H 1
#ifndef __APPLE__
# define PETSC_HAVE_SYS_PROCFS_H 1
#endif
#define PETSC_HAVE_SYS_RESOURCE_H 1
#define PETSC_HAVE_SYS_SOCKET_H 1
#ifdef __APPLE__
# define PETSC_HAVE_SYS_SYSCTL_H 1
#else
# define PETSC_HAVE_SYS_SYSINFO_H 1
#endif
#define PETSC_HAVE_SYS_TIMES_H 1
#define PETSC_HAVE_SYS_TIME_H 1
#define PETSC_HAVE_SYS_TYPES_H 1
#define PETSC_HAVE_SYS_UTSNAME_H 1
#define PETSC_HAVE_SYS_WAIT_H 1
#define PETSC_HAVE_TGAMMA 1
#define PETSC_HAVE_TIME 1
#define PETSC_HAVE_TIME_H 1
#define PETSC_HAVE_UNAME 1
#define PETSC_HAVE_UNISTD_H 1
#define PETSC_HAVE_USLEEP 1
#define PETSC_HAVE_VA_COPY 1
#define PETSC_HAVE_VSNPRINTF 1
#define PETSC_HAVE_XMMINTRIN_H 1
#define PETSC_IS_COLORING_MAX USHRT_MAX
#define PETSC_IS_COLORING_VALUE_TYPE short // NOLINT
#define PETSC_IS_COLORING_VALUE_TYPE_F integer2
#ifdef __APPLE__
# define PETSC_LEVEL1_DCACHE_LINESIZE 32
#else
# define PETSC_LEVEL1_DCACHE_LINESIZE 64
#endif
#define PETSC_LIB_DIR "NO_PETSC_LIB_DIR"
#ifdef __APPLE__
# define PETSC_MAX_PATH_LEN 1024
#else
# define PETSC_MAX_PATH_LEN 4096
#endif
#define PETSC_MEMALIGN 16
#define PETSC_MPICC_SHOW "Unavailable"
#define PETSC_MPIU_IS_COLORING_VALUE_TYPE MPI_UNSIGNED_SHORT
#define PETSC_PREFETCH_HINT_NTA _MM_HINT_NTA
#define PETSC_PREFETCH_HINT_T0 _MM_HINT_T0
#define PETSC_PREFETCH_HINT_T1 _MM_HINT_T1
#define PETSC_PREFETCH_HINT_T2 _MM_HINT_T2
#define PETSC_PYTHON_EXE "NO_PETSC_PYTHON_EXE"
#define PETSC_Prefetch(a, b, c) _mm_prefetch((const char*)(a), (c))
#define PETSC_REPLACE_DIR_SEPARATOR '\\'
#ifdef __APPLE__
# define PETSC_RTLD_DEFAULT 1
#endif
#define PETSC_SIGNAL_CAST
#define PETSC_SIZEOF_ENUM 4
#define PETSC_SIZEOF_INT 4
#define PETSC_SIZEOF_LONG 8
#define PETSC_SIZEOF_LONG_LONG 8
#define PETSC_SIZEOF_SHORT 2
#define PETSC_SIZEOF_SIZE_T 8
#define PETSC_SIZEOF_VOID_P 8
#define PETSC_SLSUFFIX "so"
#define PETSC_UINTPTR_T uintptr_t
#define PETSC_UNUSED __attribute((unused))
//#define PETSC_USE_AVX512_KERNELS 1 - Disable for future-proofing.
#define PETSC_USE_BACKWARD_LOOP 1
#define PETSC_USE_CTABLE 1
//#define PETSC_USE_DEBUGGER "gdb"
#define PETSC_USE_INFO 1
#define PETSC_USE_ISATTY 1
#define PETSC_USE_MALLOC_COALESCED 1
#ifndef __APPLE__
# define PETSC_USE_PROC_FOR_SIZE 1
#endif
#define PETSC_USE_REAL_DOUBLE 1
//#define PETSC_USE_SHARED_LIBRARIES 1
#define PETSC_USE_SINGLE_LIBRARY 1
//#define PETSC_USE_SOCKET_VIEWER 1
#define PETSC_USE_VISIBILITY_C 1
#define PETSC_USE_VISIBILITY_CXX 1
#define PETSC_USING_64BIT_PTR 1
#ifdef __APPLE__
# define PETSC_USING_DARWIN 1
#endif
//#define PETSC_USING_F2003 1 - Not using Fortran, shouldn't need this.
//#define PETSC_USING_F90FREEFORM 1 - Not using Fortran, shouldn't need this.
#define PETSC_VERSION_BRANCH_GIT "NO_PETSC_VERSION_BRANCH_GIT"
#define PETSC_VERSION_DATE_GIT "NO_PETSC_VERSION_DATE_GIT"
#define PETSC_VERSION_GIT "NO_PETSC_VERSION_GIT"
#define PETSC__BSD_SOURCE 1
#define PETSC__DEFAULT_SOURCE 1
#define PETSC__GNU_SOURCE 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 unresolved discussions, LGTM missing from assignees jwnimmer-tri(platform),xuchenhan-tri(platform), missing label for release notes (waiting on @BetsyMcPhail, @jwnimmer-tri, and @xuchenhan-tri)
tools/workspace/petsc/stubs/petscconf.h
line 185 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Drake must work on non-AVX512 machines.
IIUC, newer processors may not have AVX-512? I removed this in my latest version. (Feel free to alter the explanatory comment.)
tools/workspace/petsc/stubs/petscconf.h
line 201 at r7 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
We don't use the Fortran build.
I suspect these are harmless, but removed in my latest version.
Myself and @xuchenhan-tri will take over this upgrade. It is not an efficient use of your time @mwoehlke-kitware while the process is in its current state. We'll grab the rebased patch files from here, but we'll re-do the petscconf from scratch. Most likely, we'll improve petscconf maintenance (without any version upgrade) first in its own PR, and then do the upgrade PR in a second pass. Since this upgrade is blocked on maintainability changes, I'll close it. |
Towards #17093.
This change is