Move override-ld to printchplenv #20385
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
arezaii
approved these changes
Aug 9, 2022
mppf
force-pushed
the
move-override-ld-printchplenv
branch
from
4361598 to
c33c5a0
Compare
To avoid running 'mv' if the link command failed --- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
Only gasnet configurations set it to something other than CHPL_TARGET_CXX. --- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
At least include those from printchplenv --all --no-tidy --- Signed-off-by: Michael Ferguson <[email protected]>
Use a more comprehensive solution to get to characters that can be in a #define and use the same strategy in the test code as well as the Makefile generating it. --- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
to allow it to be set in a chplconfig --- Signed-off-by: Michael Ferguson <[email protected]>
mppf
force-pushed
the
move-override-ld-printchplenv
branch
from
6b98355 to
91b32f4
Compare
--- Signed-off-by: Michael Ferguson <[email protected]>
arezaii
approved these changes
Aug 11, 2022
--- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
--- Signed-off-by: Michael Ferguson <[email protected]>
5 tasks
mppf
added a commit
that referenced
this pull request
Aug 12, 2022
Fix problems after LLVM configuration PRs Follow-up to PRs #20396 and #20385 to address issues raised by nightly testing. * build failures in cron-xc-wb-host-prgenv-cray-failure and xc-wb.prgenv-cray configurations: don't default to CHPL_LLVM_SUPPORT=system if all the LLVM dependencies are not installed. These configurations have just `llvm-config` but not the required headers. * networking-packages failure with curl tests: update `common-quickstart.bash` to set `CHPL_LLVM=none`. The quickstart setup script can provide `CHPL_LLVM=none` or `CHPL_LLVM=system` but since we test `CHPL_LLVM=system` in most configurations, in the quickstart testing configuration, we want to test `CHPL_LLVM=none`. At the moment, the curl tests do not all work with the LLVM backend. * darwin-m1 or other Mac OS testing failure with chpl-env-gen: fixed a sed portability issue between linux and Mac OS X. Reviewed by @arezaii - thanks! - [x] On Ubuntu, if llvm-14 is installed but not llvm-14-dev * default is CHPL_LLVM=unset and CHPL_LLVM_SUPPORT=bundled * setting CHPL_LLVM=system produces an error from printchplenv about a missing header - [x] On Ubuntu, if we have the needed LLVM dependencies * default is CHPL_LLVM=system and CHPL_LLVM_SUPPORT=system - [x] `make` functions on XC with PrgEnv-cray and GCC as the host compiler - [x] `make` functions on XC with PrgEnv-cray and cray-prgenv-cray as the host compiler - [x] full local testing
ronawho
added a commit
that referenced
this pull request
Aug 24, 2022
Bring in upstream gasnet patch for checking if LD requires MPI [reviewed by @mppf] Bring in https://bitbucket.org/berkeleylab/gasnet/pull-requests/554 and some related work to enable getting `GASNET_LD_REQUIRES_MPI` out of gasnet's pkg-config file. This is a follow up to #20385, which has additional motivation for the change.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses a future work item left by PR #18880. In particular, historically we have saved the linker to use in a file
override-ldand then the LLVM backend reads this file and uses the selected linker in order to link the binary. We recently observed some problems with this strategy in the context of Homebrew. The Homebrew Chapel package had thisoverride-ldfile referring to clang in a particular location; but the Homebrew clang package was upgraded and clang was no longer available at that location. (See also Homebrew/homebrew-core#107405 ).So, this PR moves the
override-ldlogic to something supported by the chplenv Python scripts. One wrinkle with that is that the Makefile logic was usingGASNET_LD_REQUIRES_MPIbut that variable is not stored in the Gasnet .pc file that is used by the chplenv scripts in LLVM compilation. So, this PR uses the approximation of determiningGASNET_LD_REQUIRES_MPIby searching formpiin theGASNET_LDcommand name.Additionally, while debugging the issue, I noticed some odd things about the error handling within
executeAndWait, so this PR cleans that function up a bit as well. It also removes some arguments to that function that aren't really necessary. It adds a C++ dyno test of this function to make sure the return value of the function is 0 when the command runs and something other than 0 when the command does not exist.The chpl-env-gen.h needed some adjustment to work with this change so I tidied it up to stop filtering out environment variables that often have characters that can't be in a
#defineand instead to use a more comprehensive pattern when replacing such characters with_.Future Work:
GASNET_LD_REQUIRES_MPIin the logic to match the previous behavior.Reviewed by @arezaii