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

Remove themis_version() API #388

Merged
merged 5 commits into from
Feb 22, 2019
Merged

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Feb 20, 2019

As discussed in #324 and offline, remove themis_version() function and all related API for querying Themis and Soter versions at run-time. There is no replacement and this is obviously a breaking change.

This API was not really widely used and it is a bit bothersome to maintain. It is recommended to not depend on Themis version at run-time. Applications should link against a particular version of Themis during build or development (whatever is applicable to the language environment). The version should be selected by the package manager: either the system one for Themis core, or language-specific for language bindings.

themis_version() was used tests to print a version banner, that's not really important. It was also used by Rust binding to verify that FFI calls can be made, replace it with some other function. And it was also exported for Ruby programs, drop the export. The other use I'm aware of is in my Homebrew formula, that can be dealt with exactly as with Rust.

Note that the code still contains hardcoded versions in Makefile as well as in PHP and PHP7 bindings. These are going to stay for technical reasons.

Closes #324

@ilammy ilammy added W-RbThemis ♦️ Wrapper: RbThemis, Ruby API, Ruby gems core Themis Core written in C, its packages W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates labels Feb 20, 2019
Unfortunately, the status constants are defined as macros so bindgen
is not able to type them properly as themis_status_t. Do it manually.
@vixentael
Copy link
Contributor

@ilammy please grep all source and config files to check if soter.c and themis.c are not linked somewhere as exact dependencies.

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 21, 2019

@vixentael I've reviewed the code, it seems that temis.c and soter.c are not referenced anywhere explicitly. They are included into respective libraries using wildcards in makefiles:

THEMIS_SRC = $(wildcard $(SRC_PATH)/themis/*.c)
THEMIS_AUD_SRC = $(wildcard $(SRC_PATH)/themis/*.c)
THEMIS_AUD_SRC += $(wildcard $(SRC_PATH)/themis/*.h)

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 21, 2019

Now when I see DEPRECATED macro I think we can use version with pre-processor to automatically remove code after some version : ). for example DEPRECATED('msg', 0.12.0) that check with pre-processor logic that version is lower than 0.12.0 and leave code otherwise remove.

#if VERSION <= 0.12.0
  // deprecated code
#endif

https://www.slac.stanford.edu/comp/unix/gnu-info/cpp_1.html#SEC36

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 21, 2019

@Lagovas why don't we just remove the deprecated code from the files altogether with a new release? First we mark it deprecated (but still have to maintain it) and then completely remove the code at some later point.

Though I do see a merit of specifying the version in the deprecation attributes: we can mark the code as deprecated since some version and use that version as a reminder for us that some functionality has been deprecated for too long and should actually be removed.

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 21, 2019

to not maintain it manually and leave it for pre-processor if we will forget to remove legacy code and to provide explicit version when it will be removed. I saw that other libraries use deprecation warnings in some versions in a row and document in their releases what will be removed in a future and in which release it will be done. For example, it do django (python web framework), rust

@ilammy
Copy link
Collaborator Author

ilammy commented Feb 21, 2019

Hm... For that we will need some machine-readable version first.

Then we can probably invent something like this using some advanced macrology:

THEMIS_DEPRECATED(SINCE(0,11,0), UNTIL(0,12,0), AT_MOST(0,13,0),
    "use 'bar' instead"
)
void foo(void);

which will then cascade attributes:

  • expands to nothing in 0.10.0
  • adds deprecated attribute since 0.11.0 (compiler warning if the function is used)
  • adds error attribute since 0.12.0 (compiler error if the function is used)
  • hard compilation error since 0.13.0 if this code is not removed

But that looks like overengineering to me, TBH.

@Lagovas
Copy link
Collaborator

Lagovas commented Feb 21, 2019

it was just thoughts about for what we can leave version) if we decided to remove version and we don't need it, it's okay to remove

@vixentael
Copy link
Contributor

@Lagovas nice catch about deprecation macros! it might be useful for us in future.

@ilammy currently let's just remove version.
LGMT to merge

@vixentael vixentael self-requested a review February 21, 2019 21:09
@ilammy ilammy merged commit 6518d33 into cossacklabs:master Feb 22, 2019
@ilammy ilammy deleted the drop-themis-version branch February 22, 2019 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Themis Core written in C, its packages W-RbThemis ♦️ Wrapper: RbThemis, Ruby API, Ruby gems W-RustThemis 🦀 Wrapper: Rust-Themis, Rust API, Cargo crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants