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

makedep: Include object file dependencies #288

Merged
merged 1 commit into from
Dec 30, 2022

Conversation

marshallward
Copy link
Member

The current implementation of makedep contains something like a race condition, where the creation of the .mod and .o files may be in an order which breaks the current dependency tree. Currently, .mod depends on .o, and changes to a module do not trigger rebuilds of dependent source.

Rather than try to sort out the rule order, which could even depend on compiler internals, this patch just adds both object and module output files as dependencies, and rebuilds if either changes.

We might want to come back to this someday and understand the actual order of rule execution.

Thanks to Alistair Adcroft (@adcroft) for proposing this solution.

@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #288 (e713bb9) into dev/gfdl (40c24c5) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head e713bb9 differs from pull request most recent head f6db061. Consider uploading reports for the commit f6db061 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #288      +/-   ##
============================================
+ Coverage     37.07%   37.11%   +0.04%     
============================================
  Files           263      263              
  Lines         73623    73493     -130     
  Branches      13720    13700      -20     
============================================
- Hits          27296    27278      -18     
+ Misses        41290    41185     -105     
+ Partials       5037     5030       -7     
Impacted Files Coverage Δ
src/framework/MOM_document.F90 74.10% <0.00%> (-0.23%) ⬇️
...ig_src/drivers/solo_driver/MOM_surface_forcing.F90 24.42% <0.00%> (-0.11%) ⬇️
...eterizations/lateral/MOM_lateral_mixing_coeffs.F90 42.49% <0.00%> (-0.09%) ⬇️
src/tracer/MOM_tracer_hor_diff.F90 76.48% <0.00%> (-0.07%) ⬇️
src/user/Kelvin_initialization.F90 0.00% <0.00%> (ø)
src/user/dumbbell_initialization.F90 0.00% <0.00%> (ø)
src/user/tidal_bay_initialization.F90 0.00% <0.00%> (ø)
...rc/parameterizations/vertical/MOM_tidal_mixing.F90 1.40% <0.00%> (ø)
src/user/MOM_wave_interface.F90 1.32% <0.00%> (+0.01%) ⬆️
src/parameterizations/vertical/MOM_CVMix_KPP.F90 0.80% <0.00%> (+0.03%) ⬆️
... and 6 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

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

I agree with the change to the normal makefile output, but when debugging is turned on (makedep -d ...) I think it better to report both found_mods and found_deps. Could you add that debugging?

@marshallward
Copy link
Member Author

found_mods and found_objs? or do you also want found_deps?

@adcroft
Copy link
Member

adcroft commented Dec 21, 2022

I thought found_obs would be very long but I see not, so maybe that too? I just want to make sure there's enough information to understand what led to the local rule.

@marshallward
Copy link
Member Author

I just did found_mods and found_objs since found_deps is just the combination of the two

@marshallward
Copy link
Member Author

marshallward commented Dec 22, 2022

Not quite on-topic, but I noticed that Cray module files are in ALL CAPS (MOM.mod) so perhaps including the .o files is essential, just to preserve some overall functionality of the Makefile.

(Of course a better solution is to take account of the filename,but that's a separate issue...)

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

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

Looks good.

The current implementation of makedep contains something like a race
condition, where the creation of the .mod and .o files may be in an
order which breaks the current dependency tree.  Currently, .mod depends
on .o, and changes to a module do not trigger rebuilds of dependent
source.

Rather than try to sort out the rule order, which could even depend on
compiler internals, this patch just adds both object and module output
files as dependencies, and rebuilds if either changes.

We might want to come back to this someday and understand the actual
order of rule execution.

Thanks to Alistair Adcroft (@adcroft) for proposing this solution.
@Hallberg-NOAA
Copy link
Member

This PR has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/17825.

@Hallberg-NOAA Hallberg-NOAA merged commit 47fa47e into NOAA-GFDL:dev/gfdl Dec 30, 2022
@marshallward marshallward deleted the makedep_use_objs branch April 21, 2023 14:56
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