Fix various windows build issues #9641
Conversation
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
@trilinos/epetra @trilinos/teuchos @trilinos/ifpack |
jhux2
added
pkg: Epetra
pkg: Ifpack
pkg: Teuchos
|
I haven't reviewed, but I will approve to let the AT run. DO NOT MERGE until reviewed. |
bartlettroscoe
added
the
TriBITS
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jhux2 ]! |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Using Repos:
Pull Request Author: Keno |
There was a problem hiding this comment.
I am good with the TriBITS and Teuchos changes (which is what I am approving here). I personally think that the change to Epetra_C_wrappers.h is okay as well that changes to long long for 64 bit integers (but will let some @trilinos/epetra person approve that).
All of the rest of the changes all appear to be just changes to DLL exports. All of the changes are adding exports except for one case where an export is removed (for the class Ifpack_SerialTriDiSolver in the file Ifpack_SerialTriDiSolver.h). Except for that one export removal, I can't see how adding additional exports can break anything.
Others will need to review and approve the remaining changes for Epetra and Ifpack but @trilinos/ifpack developers should look carefully at the export being removed in the file Ifpack_SerialTriDiSolver.h and make sure that is okay.
NOTE: After this PR is merged, I will cherry-pick off the TriBITS change and apply it back to the TriBITS repo and snasphot TriBITS back to Trilinos to address any possible future merge conflicts.
| @@ -58,7 +58,7 @@ namespace Teuchos { | |||
| * using the non-virtual protected members <tt>setMyParamList()</tt> and | |||
| * <tt>getMyParamList()</tt>. | |||
| */ | |||
| class ParameterListAcceptorDefaultBase : virtual public ParameterListAcceptor { | |||
| class TEUCHOSPARAMETERLIST_LIB_DLL_EXPORT ParameterListAcceptorDefaultBase : virtual public ParameterListAcceptor { | |||
There was a problem hiding this comment.
Okay, we are just adding an export. I can't see how this can break an existing windows build.
| IF(${CMAKE_HOST_SYSTEM_NAME} STREQUAL "Windows") | ||
| #Apparently FIND_PROGRAM looks for an exact match of the file name. | ||
| #So even though "git clone ..." is valid to use on windows we need to give the | ||
| #full name of the command we want to run. | ||
| SET(GIT_NAME git.cmd) | ||
| ELSE(WIN32) | ||
| ELSE() | ||
| SET(GIT_NAME git) | ||
| ENDIF(WIN32) | ||
| ENDIF() |
There was a problem hiding this comment.
Okay, we are just fixing the logic for modern CMake versions on Windows and this will still use 'git' for cygwin.
| template class BLAS<long int, std::complex<float> >; | ||
| template class BLAS<long int, std::complex<double> >; | ||
| # endif | ||
| template BLAS<long int, float>; | ||
| template BLAS<long int, double>; | ||
| template class BLAS<long int, float>; | ||
| template class BLAS<long int, double>; |
There was a problem hiding this comment.
This is inside of an ifdef for _MSC_VER so Trilinos PR testing will not test these changes. But this looks like correct C++ code (and I am surprised that it compiled the way it was).
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jhux2 bartlettroscoe ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
@Keno, if no-one else approves this in the next few days, ping back here with a comment and CC those of us that commented here already. |
|
@trilinos/framework, are there any automated builds of Trilinos for Windows that might be impacted (for better or worse) by this PR? |
|
@jrobcary Does this PR affect your work? |
|
It looks innocuous, so no objection here. We have stayed with what we got working. |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
1 similar comment
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
@trilinos/framework, @trilinos/epetra, @trilinos/ifpack, @trilinos/ifpack, there is now a conflict here (likely with PR #9663 that was just merged). Unless there are objections, I propose we resolve this conflict, run another round of PR testing and merge. |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
2 similar comments
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
trilinos-autotester
added
the
AT: STALE
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
The standard syntax (and only syntax for this accepted by GCC) is `template class <instantiation>` rather than simply `template <instantitation>`. Since this is guarded by WIN32, it is possible MSVC accepts the latter as an extension (I did not check this). However, to make it build everywhere, use the standard syntax.
These were marked as being exported from epetra, but are actually in ifpack. It is possible that rather than removing these, they should get their own ifpack-export specification, but that depends on how they are used. It did not show up in my build, but in any case, the epetra export spec is clearly incorrect so needs to be removed. If they need to be exported from ifpack, that can be done in a follow up commit.
Keno
dismissed stale reviews from kddevin, bartlettroscoe, and jhux2
via
9a46378
Done |
jhux2
added
the
AT: AUTOMERGE
It marks DLL visibility for windows. Classes without this attribute will not be visible from downstream dlls. Other platforms have a similar concept, but with the opposite default. |
|
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jhux2 ]! |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Using Repos:
Pull Request Author: Keno |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_8.3.0 # 5619 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_serial # 3180 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_gcc_7.2.0_debug # 3681 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 10898 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105 # 2331 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_cuda_10.1.105_uvm_off # 1326 (click to expand)Console Output (last 100 lines) : Trilinos_pullrequest_clang_10.0.0 # 3705 (click to expand)Console Output (last 100 lines) : python-3 # 354 (click to expand) |
jhux2
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
Using Repos:
Pull Request Author: Keno |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_serial
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_7.2.0_debug
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_cuda_10.1.105_uvm_off
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_clang_10.0.0
Jenkins Parameters
Build InformationTest Name: python-3
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ jhux2 ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 9641: IS A SUCCESS - Pull Request successfully merged |
trilinos-autotester
removed
the
AT: AUTOMERGE
The prior patch pulled over from Trilinos PR trilinos/Trilinos#9641 did not use lower-case for set().
…d-logic Apply patch for windows logic from trilinos/Trilinos#9641
|
FYI: I pulled off the commit patch from commit 5dee9af and applied to the TriBITS repo in TriBITSPub/TriBITS#422 (along with fixing the command case). This will create a merge conflict on the next TriBITS snapshot back to Trilinos but it will be trivial to resolve. |
These got merged in trilinos/Trilinos#9641.
* Trilinos: Update to 14.4.0 This is the latest version. Before I try to mess with the Kokkos build (#7384), better update, since the Kokkos integration has changed a fair bit on Trilinos master. * Drop patches These got merged in trilinos/Trilinos#9641. * Use newer cmake * Factor options more nicely * Drop libgfortran3 * Correct version spelling * Add Kokkos dependency * Add patch to use external Kokkos instead * Fix Teko build * Add missing patch file * Build Kokkos-dependent libraries only if available * Forbid libgfortran4 also * Fix missing export on windows * Why do I keep forgetting the patch?
@trilinos/epetra @trilinos/teuchos @trilinos/ifpack
Motivation
Fixes a number of minor issues when building for windows (in particular a MinGW cross compile from linux - though most of the issues are applicable to MSVC also). Details of each particular change are in the commit message. With these changes, my specific configuration of Trilinos builds fully for Windows.
Testing
Automated build testing for all platforms I have access to was done in JuliaPackaging/Yggdrasil#3569. No functional changes expected.