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

Remove endfunction(<string>) and endmacro(<string>) (#274, #382) #383

Conversation

bartlettroscoe
Copy link
Member

@bartlettroscoe bartlettroscoe commented Jun 23, 2021

Resolves #382.

Follow-on to #274 and PR #379. See more details in commit e9ea581

@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented Jun 23, 2021

@gsjaardema, can you please accept the invite for write access to this repo and review and approve this PR (since this is your code)? Note that it is generally considered best practice not to include an argument in end<statement>() commands. See here and here.

…riBITSPub#382)

This is a follow-on from the lower-casing in TriBITSPub#274 with PR TriBITSPub#379.  It
lower-cased the function and macro names at the opening macro(<name> ...) and
function(<name> ...) but not the endmacro(<name> ...) and endfunction(<name>
...) calls.  (I did not realize the TriBITS had any of those still.)  Turns
out that when the text in the opening macro(<text>) and function(<text>) does
not exactly match the endmacro(<text>) and endfunction(<text>), you get a
nasty warning message:

  A logical block opening on the line ... closes on the line ... with
  mis-matching arguments.

(see TriBITSPub#382).

The TriBITS test suite does not actually run any of this code so this was
never seen prior to the creation of TriBITSPub#382.  Therefore, I don't know that this
solves the problem but I strongly suspect that it will.  (I will run a test
with an ATDM Trilinos build that should trigger this code.)
@bartlettroscoe bartlettroscoe force-pushed the 382-remove-endfunction-endmacro-strings branch from 4bfd47b to e9ea581 Compare June 23, 2021 16:57
@bartlettroscoe
Copy link
Member Author

With the ATDM Trilinos 'cee-rhel7-clang-opt' build on the machine 'ceerws1113' using:

$ cd /scratch/rabartl/Trilinos.base/BUILDS/ATDM/CEE-RHEL7/CHECKIN/cee-rhel7-clang-opt/

$ cat load-env.sh 
source /scratch/rabartl/Trilinos.base/Trilinos/cmake/std/atdm/load-env.sh cee-rhel7-clang-opt

$ . load-env.sh 
Hostname 'ceerws1113' matches known ATDM host 'ceerws1113' and system 'cee-rhel7'
Setting compiler and build options for build-name 'cee-rhel7-clang-opt'
Using CEE RHEL7 compiler stack CLANG-9.0.1_OPENMPI-4.0.3 to build RELEASE code with Kokkos node type SERIAL

$ cat do-configure.base
cmake \
-GNinja \
-DTrilinos_TRIBITS_DIR:STRING=TriBITS/tribits \
-DTrilinos_ENABLE_TESTS:BOOL=ON \
-DTrilinos_TEST_CATEGORIES:STRING=NIGHTLY \
-DTrilinos_ALLOW_NO_PACKAGES:BOOL=OFF \
-DDART_TESTING_TIMEOUT:STRING=600.0 \
-GNinja \
-DTrilinos_CONFIGURE_OPTIONS_FILE:STRING=cmake/std/atdm/ATDMDevEnv.cmake \
-DTrilinos_TRACE_ADD_TEST=ON \
"$@" \
/scratch/rabartl/Trilinos.base/Trilinos/cmake/std/atdm/../../..

$ cat do-configure
./do-configure.base \
-DTrilinos_ENABLE_SEACAS:BOOL=ON \
-DTrilinos_ENABLE_ALL_OPTIONAL_PACKAGES:BOOL=ON \
-DTrilinos_ENABLE_ALL_FORWARD_DEP_PACKAGES:BOOL=OFF \
"$@"

$ rm -r CMake*

$ ./do-configure &> configure.out

$ grep "CMake Warning" configure.out | wc -l
14

I was able to reproduce the warnings reported in #382 showing, for example:

-- Using find_package(Netcdf ...) ...
CMake Warning (dev) in TriBITS/tribits/common_tpls/utils/ParseLibraryList.cmake:
  A logical block opening on the line

    /scratch/rabartl/Trilinos.base/Trilinos/TriBITS/tribits/common_tpls/utils/ParseLibraryList.cmake:51 (function)

  closes on the line

    /scratch/rabartl/Trilinos.base/Trilinos/TriBITS/tribits/common_tpls/utils/ParseLibraryList.cmake:115 (endfunction)

  with mis-matching arguments.
Call Stack (most recent call first):
  TriBITS/tribits/common_tpls/utils/AddPackageDependency.cmake:51 (include)
  TriBITS/tribits/common_tpls/find_modules/FindNetCDF.cmake:72 (include)
  TriBITS/tribits/common_tpls/FindTPLNetcdf.cmake:66 (find_package)
  TriBITS/tribits/core/package_arch/TribitsProcessEnabledTpl.cmake:106 (include)
  TriBITS/tribits/core/package_arch/TribitsGlobalMacros.cmake:1604 (tribits_process_enabled_tpl)
  TriBITS/tribits/core/package_arch/TribitsProjectImpl.cmake:196 (tribits_process_enabled_tpls)
  TriBITS/tribits/core/package_arch/TribitsProject.cmake:93 (tribits_project_impl)
  CMakeLists.txt:109 (TRIBITS_PROJECT)
This warning is for project developers.  Use -Wno-dev to suppress it.

Then switching over the the version of TriBITS in this PR branch 'checkout 382-remove-endfunction-endmacro-strings', I again ran:

$ rm -r CMake*

$ ./do-configure &> configure.out

$ tail -n 5 configure.out
Finished configuring Trilinos!

-- Configuring done
-- Generating done
-- Build files have been written to: /scratch/rabartl/Trilinos.base/BUILDS/ATDM/CEE-RHEL7/CHECKIN/cee-rhel7-clang-opt

$ grep "CMake Warning" configure.out | wc -l
0

So I think this should be good to merge.

@bartlettroscoe bartlettroscoe merged commit d17fbf3 into TriBITSPub:master Jun 23, 2021
@gsjaardema
Copy link
Contributor

@bartlettroscoe Looks good. Sorry for the delay, did not see this until now.

bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this pull request Sep 4, 2021
Brings in numerous refactorings to TriBITS over the last 3 months, but there
should be no breaks in backward compatibility.  Almost every file in TriBITS
is changed due to the lower-casing of command, macro and function names in PR
TriBITSPub/TriBITS#379.  But the main driver for this snapshot is to bring in
the change in PR TriBITSPub/TriBITS#413 that should make it so that Kokkos
INTERFACE_COMPILE_OPTIONS get propagated to downstream targets in TriBITS and
therefore to external customers through installed <Package>Config.cmake files
and IMPORTED targets.  I should have done several snapshots in the last few
months and not done a big snapshot like this (but I have been testing with
Trilinos locally along the way).

Overall, this merge brings in changes from a bunch of TriBITS PRs including
(from most recent):

* TriBITSPub/TriBITS#413: Change internal TriBITS target_link_libraries() to
  PUBLIC (TriBITSPub/TriBITS#299) component: core type: enhancement

* TriBITSPub/TriBITS#410: Upgrade from cmake 3.21.0 to 3.21.2
  (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394)

* TriBITSPub/TriBITS#394: DO NOT MERGE: Show TriBITS test failures with CMake
  3.21.0 that don't occur with CMake 3.17.5 (TriBITSPub/TriBITS#363)

* TriBITSPub/TriBITS#409: Add getTestDictStatusField() to handle empty
  'status' field (SESW-383) component: ci_support type: enhancement

* TriBITSPub/TriBITS#408: Deal with spaces in CDash url parts (SESW-383)
  component: ci_support type: enhancement

* TriBITSPub/TriBITS#403: Spelling fixes

* TriBITSPub/TriBITS#407: Move tribits_get_build_url_and_write_to_file() to
  stand-alone module (TriBITSPub/TriBITS#154) component: ctest_driver type:
  enhancement

* TriBITSPub/TriBITS#388: Fixing typos (TriBITSPub/TriBITS#377)

* TriBITSPub/TriBITS#406: Fix tribits_ctest_driver() package-by-package mode
  for CMake 3.19+ (TriBITSPub/TriBITS#363, TriBITSPub/TriBITS#394) component:
  ctest_driver type: bug

* TriBITSPub/TriBITS#401: Improve GitHub Actions and CDash integration
  (TriBITSPub/TriBITS#154) type: enhancement

* TriBITSPub/TriBITS#366: CI: draft action yaml

* TriBITSPub/TriBITS#402: Revert some incorrect uppercase->lowercase changes

* TriBITSPub/TriBITS#387: Build and deploy TriBITS documentation with Sphinx
  (TriBITSPub/TriBITS#386) component: documentation type: enhancement

* TriBITSPub/TriBITS#398: Deal with pr null in not preprending build name
  (TriBITSPub/TriBITS#363) type: bug

* TriBITSPub/TriBITS#396: Send PR results to 'Pull Request' CDash group and
  add PR ID to build names (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#397: Print the ninja path and version
  (TriBITSPub/TriBITS#363)

* TriBITSPub/TriBITS#393: GitHub Actions based testing for TriBITS
  (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#389: TriBITS CI testing with GitHub Actions
  (TriBITSPub/TriBITS#363) type: enhancement

* TriBITSPub/TriBITS#392: Fix broken tests for non-Fortran and CMake 3.21
  builds (TriBITSPub/TriBITS#363) component: core

* TriBITSPub/TriBITS#391: Fix up build_docs.sh for Sphinx doc generation
  (TriBITSPub/TriBITS#386) component: documentation type: enhancement

* TriBITSPub/TriBITS#390: Add test for doc generation and fix usage of Python3
  component: documentation type: bug

* TriBITSPub/TriBITS#385: Replace last few references to
  TribitsDevelopersGuide.html (TriBITSPub/TriBITS#381) component:
  documentation type: enhancement

* TriBITSPub/TriBITS#384: Split TribitsDevelopersGuide.* into
  TribitsUsersGuide.* and TribitsMaintainersGuide.* (TriBITSPub/TriBITS#381)
  component: documentation type: enhancement

* TriBITSPub/TriBITS#383: Remove endfunction(<string>) and endmacro(<string>)
  (TriBITSPub/TriBITS#274, TriBITSPub/TriBITS#382) component: common_tpls
  type: bug

* TriBITSPub/TriBITS#380: More package-arch data-structure documentation
  updates (TriBITSPub/TriBITS#63) component: documentation type: enhancement

* TriBITSPub/TriBITS#379: Convert TriBITS to lower-case CMake command, macro,
  and function names (TriBITSPub/TriBITS#274)

The following files were conflicting where I went with what is on the Trilinos
'develop' branch:

* cmake/tribits/common_tpls/FindTPLBLAS.cmake
* cmake/tribits/common_tpls/FindTPLLAPACK.cmake
* cmake/tribits/common_tpls/FindTPLNetcdf.cmake

(It looks like the above changes never made it back into TriBITS proper.  The
conflicts were due to the case changes in cmake command calls in these files
due to TriBITSPub/TriBITS#379.)

There was also a conflict in the file:

* cmake/tribits/core/installation/TribitsProjectConfigTemplate.cmake.in

I looked at that one carefully and I think that may have been due to fixes on
both sides and then the case changes from TriBITSPub/TriBITS#379.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

new warnings during charon configuration
2 participants