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

Infrastructure Improvements #610

Merged
merged 5 commits into from
Dec 11, 2020
Merged

Conversation

rafmudaf
Copy link
Collaborator

Feature or improvement description
This pull request does a few minor things:

  • Adds some resolution for when to run the GH Actions workflow (i.e. don't compile and run tests when the only thing that changed is the documentation)
  • Removes unused CI configuration files (au revoir, Travis)
  • Prevents variable tracking for large Fortran modules

Impacted areas of the software
Testing and CMake

Additional supporting information
One question is whether disabling variable tracking should only happen in Release mode. @andrew-platt have any thoughts? Relevant documentation for GNU Fortran is here. My thought is that if -g is on, we probably want this enabled.

@rafmudaf rafmudaf self-assigned this Dec 10, 2020
add_executable(openfast
src/FAST_Prog.f90)
add_executable(openfast src/FAST_Prog.f90)
set_source_files_properties(src/FAST_Prog.f90 PROPERTIES COMPILE_FLAGS "-fno-var-tracking -fno-var-tracking-assignments")
Copy link
Contributor

@sayerhs sayerhs Dec 10, 2020

Choose a reason for hiding this comment

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

heh... I was just griping about this on eagle. Great addition!

I see this also in FAST_Types and FAST_Subs

modules/openfast-libra
ry/src/FAST_Types.f90:46498:0:

 END MODULE FAST_Types

note: variable tracking size limit exceeded with -fvar-tracking-assignments, retrying without```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I've also added this to both of those modules.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sayerhs do you have any input on whether this should always be disabled or if only for Release mode (-O3)? I think I see this warning with -O2 as well, but RelWithDebInfo includes -g so I think we may want variable tracking there.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should use CMake conditional syntax to apply this only if the CMAKE_Fortran_COMPILER_ID is GCC

Copy link
Contributor

Choose a reason for hiding this comment

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

@rafmudaf My vote would be to not add -fno-var-tracking... flags for Debug build, but use it for Release and RelWithDebInfo. With -O2 -g a lot of variables are removed anyway, so I am not sure it is going to affect much.

Copy link
Collaborator

@andrew-platt andrew-platt left a comment

Choose a reason for hiding this comment

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

Are the compile flags "-fno-var-tracking -fno-var-tracking-assignments" specific to the gfortran compiler? Will this be an issue with windows + ifort?

@andrew-platt
Copy link
Collaborator

I think we should only add the no-var-tracking when not using the -g option. Probably safe to do for -O2 and higher.

@rafmudaf
Copy link
Collaborator Author

Ok I've added a check for GNU compiler and CMAKE_BUILD_TYPE=Release mode.

@rafmudaf
Copy link
Collaborator Author

I've left variable tracking enabled for RelWithDebInfo for now just to be conservative. We can easily add that to the if-statement, if needed.

@rafmudaf rafmudaf merged commit 4b5559f into OpenFAST:dev Dec 11, 2020
@rafmudaf rafmudaf deleted the infrastructure_updates branch December 11, 2020 17:03
@rafmudaf rafmudaf mentioned this pull request Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants