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

Deprecate legacy API [AP-643] #1373

Merged
merged 19 commits into from
Dec 19, 2023
Merged

Conversation

woodfell
Copy link
Contributor

@woodfell woodfell commented Nov 13, 2023

Description

@swift-nav/devinfra

Follow through on legacy API deprecation plan as previously agreed upon. The goals of this PR are:

  • Keep the legacy API in place but make it much more difficult to access
    • Any usage of the legacy API must be explicit on behalf of the user
    • Legacy API no longer implicitly available by including libsbp/sbp.h - legacy headers must be explicitly included
  • All header files in the legacy API generate a compile time message at the point of inclusion warning that they will be removed in version 6
  • All symbols in the legacy API are marked with the deprecated attribute
  • All mentions of v4 (ie, the modern replacement API) removed from headers, source files, etc. Moving forwards there will be no such thing as the "v4" API, there will just be the libsbp API
  • Shuffle files around in the source and include directories to better reflect the transition from legacy to modern APIs
    • The correct way to include a package worth of message types is by including (eg) libsbp/observation.h
    • v4 "package" header files (eg libsbp/v4/observation.h) are still available but are deprecated. They generate a compile time message warning that they will be removed in version 6 and then include libsbp/<package>.h (the new header file) automatically
  • The end goal of all of this is that there is the libsbp API (previously know as the v4 API), plus a legacy API which is hanging around but very much a second class citizen.
  • The legacy code is sufficiently separated so that removing it in version 6 should be a fairly trivial task consisting mostly of deleting some source/header files and templates

API compatibility

Does this change introduce a API compatibility risk?

Nothing significant. The vast majority of symbols are present and have the same meaning as before. Users of the legacy API may find that they need to alter some include directives, but the warnings should be obnoxious enough that they have no excuse for not transitioning to the modern API immediately.

The one piece which has changed is the C++ state wrapper (sbp::State) which previously was capable of handling both the legacy and modern APIs. This has been broken up so that sbp::State now only handles the modern API, and a new class sbp::LegacyState inherits from sbp::State and adds a coupld of functions to deal with the legacy API. This class is immediately deprecated and marked for deletion in version 6

The modern API is untouched by this PR

API compatibility plan

If the above is "Yes", please detail the compatibility (or migration) plan:

All users must transition to the modern API immediately if they haven't already done so.

JIRA Reference

https://swift-nav.atlassian.net/browse/AP-643

@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from a04cd75 to 5c31084 Compare November 14, 2023 10:00
@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from 5c31084 to 9f0a61b Compare November 23, 2023 05:04
@woodfell woodfell changed the title Woodfell/follow v5 deprecation plan Deprecate legacy API [AP-643] Nov 23, 2023
@woodfell woodfell marked this pull request as ready for review November 23, 2023 06:06
@woodfell woodfell requested review from jungleraptor and a team as code owners November 23, 2023 06:06
@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from f10fe9f to 2b54f61 Compare November 23, 2023 06:07
Copy link
Contributor

@dgburr dgburr left a comment

Choose a reason for hiding this comment

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

I tested the changes from the woodfell/follow_v5_deprecation_plan with a couple of apps which were written against the v4 API and everything seemed to work as expected:

[removed internal links]

I confirmed in both cases that it was possible to build with the new "default" API as well as by explicitly requesting the v4 API.

I then checked with some other demo code which was written against the v3 API and confirmed that they can still be compiled with the libsbp/legacy/compat.h header. However, when I tried to adapt these examples to the newer API, I found that there does not appear to be any direct replacement for sbp_send_message()/sbp_payload_send() in the new API. @woodfell: what is the modern equivalent?

Lastly, I would like to ask that we provide updated versions of tcp_example/tcp_2sigma_example with the new API. Otherwise these examples will get lost when the legacy API is removed. I can help out here if necessary.

@woodfell
Copy link
Contributor Author

woodfell commented Nov 24, 2023

I tested the changes from the woodfell/follow_v5_deprecation_plan with a couple of apps which were written against the v4 API and everything seemed to work as expected:

[removed internal links]

I confirmed in both cases that it was possible to build with the new "default" API as well as by explicitly requesting the v4 API.

I then checked with some other demo code which was written against the v3 API and confirmed that they can still be compiled with the libsbp/legacy/compat.h header. However, when I tried to adapt these examples to the newer API, I found that there does not appear to be any direct replacement for sbp_send_message()/sbp_payload_send() in the new API. @woodfell: what is the modern equivalent?

Lastly, I would like to ask that we provide updated versions of tcp_example/tcp_2sigma_example with the new API. Otherwise these examples will get lost when the legacy API is removed. I can help out here if necessary.

Oh right, I thought there were already duplicates of all the examples. I'll add them on to this PR

Copy link
Contributor

@pcrumley pcrumley left a comment

Choose a reason for hiding this comment

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

@woodfell I guess i'm kind of surprised these changes only affect the C code in libsbp.

I would think it would also affect the other language implementations of libsbp. If so, i think it would be a good idea to generate all those updates before doing a release (although maybe not part of this PR)

@pcrumley
Copy link
Contributor

@woodfell The build docker step is failing with an npm version error:

2023-12-09T05:03:34.4395394Z Installing Javascript dependencies ...
2023-12-09T05:03:34.4407772Z 
2023-12-09T05:03:34.4410289Z cd /mnt/workspace; npm install
2023-12-09T05:03:34.7306187Z ERROR: npm v10.2.5 is known not to run on Node.js v14.17.3.  This version of npm supports the following node versions: `^18.17.0 || >=20.5.0`. You can find the latest version at https://nodejs.org/.
2023-12-09T05:03:34.7307826Z 
2023-12-09T05:03:34.7307973Z ERROR:
2023-12-09T05:03:34.7322037Z /opt/nvm/versions/node/v14.17.3/lib/node_modules/npm/node_modules/@npmcli/agent/lib/agents.js:105
2023-12-09T05:03:34.7323382Z     options.lookup ??= this.#options.lookup
2023-12-09T05:03:34.7323906Z                    ^^^
2023-12-09T05:03:34.7324162Z 
2023-12-09T05:03:34.7324590Z SyntaxError: Unexpected token '??='
2023-12-09T05:03:34.7325373Z �[90m    at wrapSafe (internal/modules/cjs/loader.js:1001:16)�[39m
2023-12-09T05:03:34.7326532Z �[90m    at Module._compile (internal/modules/cjs/loader.js:1049:27)�[39m
2023-12-09T05:03:34.7327956Z �[90m    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)�[39m
2023-12-09T05:03:34.7329329Z �[90m    at Module.load (internal/modules/cjs/loader.js:950:32)�[39m
2023-12-09T05:03:34.7330665Z �[90m    at Function.Module._load (internal/modules/cjs/loader.js:790:14)�[39m
2023-12-09T05:03:34.7331736Z �[90m    at Module.require (internal/modules/cjs/loader.js:974:19)�[39m
2023-12-09T05:03:34.7332876Z �[90m    at require (internal/modules/cjs/helpers.js:92:18)�[39m
2023-12-09T05:03:34.7334617Z     at Object.<anonymous> (/opt/nvm/versions/node/v14.17.3/lib/node_modules/�[4mnpm�[24m/node_modules/�[4m@npmcli�[24m/agent/lib/index.js:7:15)
2023-12-09T05:03:34.7336139Z �[90m    at Module._compile (internal/modules/cjs/loader.js:1085:14)�[39m
2023-12-09T05:03:34.7337267Z �[90m    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)�[39m

@woodfell
Copy link
Contributor Author

@woodfell I guess i'm kind of surprised these changes only affect the C code in libsbp.

I would think it would also affect the other language implementations of libsbp. If so, i think it would be a good idea to generate all those updates before doing a release (although maybe not part of this PR)

There is no legacy or V4 API in other languages. This is only applicable to C bindings as introduced in #1000

@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from ebc237b to 8225f08 Compare December 13, 2023 23:31
woodfell added a commit that referenced this pull request Dec 18, 2023
# Description

@swift-nav/devinfra

Bump node.js version in docker image to 18.17.0

This resolves the docker image build failures seen on #1373 and allows a
freshly built docker image to build the javascript bindings

# API compatibility

Does this change introduce a API compatibility risk?

No

## API compatibility plan

If the above is "Yes", please detail the compatibility (or migration)
plan:

N/A

# JIRA Reference

https://swift-nav.atlassian.net/browse/AP-1081
@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from 8225f08 to 6dd431a Compare December 19, 2023 02:53
woodfell and others added 10 commits December 19, 2023 17:55
# Description

@swift-nav/devinfra

<!-- Changes proposed in this PR -->

# API compatibility

Does this change introduce a API compatibility risk?

<!-- Provide a short explanation why or why not -->

## API compatibility plan

If the above is "Yes", please detail the compatibility (or migration)
plan:

<!-- Provide a short explanation plan here -->

# JIRA Reference

https://swift-nav.atlassian.net/browse/BOARD-XXXX
@woodfell woodfell force-pushed the woodfell/follow_v5_deprecation_plan branch from 6dd431a to 72145c7 Compare December 19, 2023 06:56
Copy link

Quality Gate Passed Quality Gate passed for 'libsbp-c'

The SonarCloud Quality Gate passed, but some issues were introduced.

184 New issues
0 Security Hotspots
92.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@woodfell woodfell merged commit b9860dc into master Dec 19, 2023
21 checks passed
@woodfell woodfell deleted the woodfell/follow_v5_deprecation_plan branch December 19, 2023 08:27
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.

4 participants