Deprecating code

[nilsvu]

Feature request:

This issue is to discuss our policy for deprecating code. It came up in this comment thread: https://github.com/sxs-collaboration/spectre/pull/1817#discussion_r349931786

This is my suggestion from #1817:

• When appropriate, prefer adding the deprecated attribute instead of deleting code (see https://en.cppreference.com/w/cpp/language/attributes/deprecated).
• The intention is to not break anyone else’s code when they rebase. Instead, they get a compiler warning on their own machine, and when submitting a PR they get CI failures so we don’t merge deprecated code.
• Note that Clang currently doesn't warn about deprecated type alias templates, but GCC does.

Component:

• [x] Code
• [ ] Documentation
• [ ] Build system
• [x] Continuous integration

[nilsvu]

Re https://github.com/sxs-collaboration/spectre/pull/1817#discussion_r349952589:

• I suppose the timescale to remove deprecated code should be some percentile of the lifetime distribution of feature branches. Are perhaps >90% of feature branches merged within a month or two? If they aren't rebased within that time, enough has changed in the code that they're probably outdated for other reasons, anyway.
• Is there any harm in keeping deprecated code around for longer, until someone bothers to remove it? Code that uses deprecated code won't pass CI, so we don't accidentally merge it.
• Once someone puts in a PR to remove deprecated code, we can check if it has been deprecated for long enough. I suppose that check could be automated, but not sure if it's worth the effort or robust enough (check modified and deleted code in PRs for deprecated keywords, somehow retrieve the commit that added the keyword, retrieve the corresponding merge commit and compare its date).

[nilsdeppe]

You're going to love this idea ;)

So we could have a macro roughly as follows:

/// MESSAGE is the deprecation message
/// DATE is the date on which this is being deprecated
#define DEPRECATED(MESSAGE, DATE) \
  BOOST_PP_IF(BOOST_PP_GREATER(CURRENT_DATE,
      DATE + 6 /* need to do some math here to get month right */),
    static_assert(false, "remove deprecated code!");, [[deprecated, MESSAGE]])

This will cause compile failures six months after code is deprecated making it clear that the code has been deprecated long enough to be removed. The downside is that on the first of every month we could have builds break and someone needs to fix it, which is kind of annoying.

An alternative mechanism would be:

#define DEPRECATED(DATE, MESSAGE) [[deprecated, MESSAGE]]

and have a CI check run only on develop flag old code either by (in order of increasing difficulty):

• Failing the build
• Filing an issue on GitHub (I'm not sure how to do this, but GitHub's REST API does support this kind of thing)
• Have CI auto-remove code (this seems horribly difficult)

[nilsvu]

Yay more macros :)

• It would probably be quite annoying to have builds break from one day to another. I suppose we could enable this only on CI and only on develop which would OK. Keep in mind that this would also break caching on CI though, because caches are only uploaded on successful CI builds. So PRs wouldn't receive up-to-date caches until the develop builds start passing again.
• I like the idea of automatically filing issues! That shouldn't be too difficult to accomplish actually.

[nilsdeppe]

I knew you'd love it ;)

• Yes, I think only breaking on develop CI builds is somewhat acceptable since then we at least have a few days to fix it. Most of the year this won't matter, basically just around holidays that fall on the 1st or so of the month.
• Filing an issue on GH automatically is well outside my realm of expertise. We already have an sxs-bot account that could be used as the creator of the issue. We do need to make sure that nobody can get access to any secret tokens by changing something on a fork or PR, but I suspect that GH actions supports that.

[nilsvu]

• GitHub Actions carry a token, so I think they can just file issues. Here's an action someone wrote that could be useful: https://github.com/JasonEtco/create-an-issue
• By default, issues would also be filed on forks that have actions enabled. I suppose this could be disabled by making it opt-in, e.g. by requiring a particular variable must be defined in the repo.

[nilsdeppe]

• Neat, looks relatively straight forward
• Is there a "main repo only" filter like there are branch filters?

[nilsvu]

You can always hard-code a repo URL in the workflow and check the ${{ github.repository }} against it. But I suppose the idea is that actions run on forks the same way they do on the base repo.