Replace NonCopyable uses with explicitly deleted ctors

[marcosbento]

Description

An attempt to modernize the code base, by removing the use of the "old style" NonCopyable in favour of explicitly deleting copy/move ctor/assignment operators. Uses of OnlyMovable are also removed, in favour of explicitly deleted/defaulted copy/move ctor/assignment operators.

This follows the advice of the CppCoreGuidelines, which states = delete is preferred -- see the discussion here.

Important: Since downstream project use NonCopyable through transitive #include dependencies, the proposed changes effectively cause build failures -- i.e. older version of odc, fdb, gribjump, ... won't be able to build against versions of eckit with these changes!

Contributor Declaration

By opening this pull request, I affirm the following:

• All authors agree to the Contributor License Agreement.
• The code follows the project's coding standards.
• I have performed self-review and added comments where needed.
• I have added or updated tests to verify that my changes are effective and functional.
• I have run all existing tests and confirmed they pass.

🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-238

[codecov-commenter]

Codecov Report

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.24%. Comparing base (f8ad909) to head (da1ba2a).

Files with missing lines	Patch %	Lines
src/eckit/io/cluster/ClusterDisks.cc	0.00%	1 Missing ⚠️
src/eckit/runtime/Monitor.h	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #238      +/-   ##
===========================================
- Coverage    66.25%   66.24%   -0.01%     
===========================================
  Files         1131     1130       -1     
  Lines        57959    57955       -4     
  Branches      4394     4394              
===========================================
- Hits         38398    38394       -4     
  Misses       19561    19561              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wdeconinck]

Compilation errors detected for ODC:
https://github.com/ecmwf/eckit/actions/runs/19869433903/job/56941583645?pr=238#step:3:2931

In file included from /opt/actions-runner/work/_work/eckit/eckit/src/odc/ODBAPISettings.cc:24:
/opt/actions-runner/work/_work/eckit/eckit/src/odc/ODBAPISettings.h:24:39: error: expected class name
24 | class ODBAPISettings : private eckit::NonCopyable {
   |                                       ^
1 error generated.

[marcosbento]

Compilation errors detected for ODC: https://github.com/ecmwf/eckit/actions/runs/19869433903/job/56941583645?pr=238#step:3:2931

In file included from /opt/actions-runner/work/_work/eckit/eckit/src/odc/ODBAPISettings.cc:24:
/opt/actions-runner/work/_work/eckit/eckit/src/odc/ODBAPISettings.h:24:39: error: expected class name
24 | class ODBAPISettings : private eckit::NonCopyable {
   |                                       ^
1 error generated.

It seems that ODC, and maybe a few others, depend on NonCopyable through transitive #include dependencies (which have now disappeared).

I feel the best way to fix this is to create PRs on the failing downstream projects to introduce the needed #include.
Even better, and since we have to touch these sources anyway, would be to simply remove the use NonCopyable from those projects and avoid the deprecation warning.

[marcosbento]

The downstream changes to fix ODC are at ecmwf/odc#48.

[simondsmart]

Thanks. Have merged the changes in odc.

[marcosbento]

Similar issues (e.g. transitive #include dependencies on NonCopyable) found in FDB are fixed and awaiting review: ecmwf/fdb#207

[marcosbento]

Following a comment from @pmaciel, the non-copyable classes now follow the rule of 5 (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rc-five).

n.b. this means only that the classes that were updated, by removing inheritance of NonCopyable, have been made to follow this rule. It is likely that other classes in eckit do not strictly adhere to the rule of 5.

[marcosbento]

Similar issues (e.g. transitive #include dependencies on NonCopyable) found in GribJump are fixed and awaiting review: ecmwf/gribjump#79

[Ozaq]

@marcosbento can you please create a eckit jira issue to track the items for 2.0?

[wdeconinck]

It is a good idea to identify all issues now and make fixes in all packages, but could we not also keep a deprecation so that existing packages can transition independently? I am thinking about being able to update eckit and eg perhaps multio in ifs-bundle (including older cycles ) without needing to update everything else. What would be required to achieve this?

[marcosbento]

It is a good idea to identify all issues now and make fixes in all packages, but could we not also keep a deprecation so that existing packages can transition independently? I am thinking about being able to update eckit and eg perhaps multio in ifs-bundle (including older cycles ) without needing to update everything else. What would be required to achieve this?

@wdeconinck To deprecate NonCopyable in eckit was my original idea. I said to myself "this should be easy pickings!".

But removing the use of NonCopyable from eckit itself caused downstream CI failures, as these packages had transitive dependencies (e.g. they included some Factory.h, and then used NonCopyable because it was made available without actually including NonCopyable.h).

The only way to still allow downstream packages to compile, would be to leave the "unnecessary" #include ".../NonCopyable.h" in eckit itself. We could certainly leave these in, but I would argue that it goes against the spirit of deprecating NonCopyable. Removing them, regardless if we do it now or later, is a clear API breaking change.

So, for me, the mission (should we choose to accept it) is to remove all uses of NonCopyable from downstream packages; and leave this PR to be merged somewhere in the future, where we have a "major" version release of eckit. This way, the adoption of the new version of eckit by the downstream packages is a essentially no-op (at least regarding NonCopyable).

[marcosbento]

@marcosbento can you please create a eckit jira issue to track the items for 2.0?

Done! See ECKIT-645...

[Ozaq]

It is a good idea to identify all issues now and make fixes in all packages, but could we not also keep a deprecation so that existing packages can transition independently? I am thinking about being able to update eckit and eg perhaps multio in ifs-bundle (including older cycles ) without needing to update everything else. What would be required to achieve this?

This is, given my understanding, the goal.

We want to remove use of 'non-copyable' in as many projects as we can without requiring additional coordination, then remove its use in eckit. Then we can declare it deprecated.

[simondsmart]

Agreed that this is the goal. Removing this class should be one of the items on the list of breaking API changes for eckit 2.0.0, due in 2026

[wdeconinck]

I understand that relying on transitive includes is a bug and not a deprecation issue in e.g. odc and therefore it is indeed a hotfix to odc to make that problem disappear. A hotfix is in those projects is warranted I think for this, for the case where eckit makes a release before a SR including odc.

[pmaciel]

Can we declare NonCopyable deprecated?

[marcosbento]

Can we declare NonCopyable deprecated?

@Ozaq suggested to remove the [[deprecated]] attribute to make the PR more focused, i.e. the focus now is "simply" to remove the uses of NonCopyable from eckit itself.

Once this PR is accepted, I'm glad to create a standalone PR just to add the [[deprecated]].

[danovaro]

Similar issues (e.g. transitive #include dependencies on NonCopyable) found in FDB are fixed and awaiting review: ecmwf/fdb#207

merged

[marcosbento]

We now have a ✅ on the build!