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

Add release notes for Trilinos 14.0 and break in backward compatibility with -isystem include dirs #6

Conversation

…ty with -isystem include dirs

Hopefully this will be enough for Trilinos users to work through problems with
changes in the handling of include directories.

For more details, see TriBITSPub/TriBITS#443
Copy link

@marcinwrobel1986 marcinwrobel1986 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No typos, good to go!

@bartlettroscoe
Copy link
Owner Author

I will need to wait to merge this to my branch 'tribits-299-modern-cmake-targets-1-again' until after I updated the TriBITS snapshot in that branch.

@bartlettroscoe bartlettroscoe requested a review from mperrinel March 4, 2022 15:10
Copy link
Collaborator

@mperrinel mperrinel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem and the different approaches are well explained.
I didn't see any typo mistake.
Ok for me.

Copy link
Owner Author

@bartlettroscoe bartlettroscoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found at least 9 typos or other problems with this text, just from reading it less than 24 hours later. I a guarantee that I have missed some other problems because I still remember what I think I wrote.

RELEASE_NOTES Outdated
critical compiler options and include directories (and other modern CMake
usage requirements) are propagated through linking to the IMPORTED library
targets in downstream customer CMake projects. This makes the usage of
the variables `<Trilinos>_INCLUDE_DIRS` and `Trilinos__TPL_INCLUDE_DIRS`
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an extra _ in Trilinos__TPL_INCLUDE_DIRS. That is an obvious typo.

RELEASE_NOTES Outdated
some fragile build environments that have the same header file names in
multiple include directories searched by the compiler. Also, this will
silence any regular compiler warnings from headers found under these
include directories. **This constitutes a break in backward compatibility
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the matching ** for the bolding? That is an obvious error.

@bartlettroscoe
Copy link
Owner Author

@marcinwrobel1986 wrote:

No typos, good to go!

@mperrinel wrote:

I didn't see any typo mistake.

Guys, a simple rereading of this this morning and I found at least 9 typos or other errors as listed in my review above.

@bartlettroscoe bartlettroscoe force-pushed the tribits-299-modern-cmake-targets-1-again-doc branch from 2d0bc1c to 6ab23b5 Compare March 4, 2022 18:20
@bartlettroscoe
Copy link
Owner Author

I fixed the typos and other problems I found above in the new commit 6ab23b5 that I just pushed.

I think the next step is to have Trilinos developers review this if they want.

@bartlettroscoe bartlettroscoe merged commit 3281be9 into tribits-299-modern-cmake-targets-1-again Mar 7, 2022
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.

3 participants