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 common 3rd party headers and update code rules #90

Merged
merged 3 commits into from
Jan 27, 2024

Conversation

L1ghtmann
Copy link
Member

@L1ghtmann L1ghtmann commented Nov 29, 2023

What does this implement/fix? Explain your changes.

  • Removes openssl and libarchive headers
  • Code rules updated to advise against including common 3rd party headers available from cross-platform dev packages

Checklist

  • New code follows the code rules
  • I've added modulemaps for any new libraries (e.g. see libactivator/module.modulemap): it should be possible to @import MyLibrary; in ObjC, or import MyLibrary in Swift.
  • My contribution is code written by myself from reverse-engineered headers, licensed into the Public Domain as per LICENSE.md; or, code written by myself / taken from an existing project, released under an OSI-approved license, and I've added relevant licensing credit to LICENSE.md

Does this close any currently open issues?

#73

Any relevant logs, error output, etc?

Any other comments?

  • Potentially breaking change
  • Headers are provided by respect *-dev packages on Linux and respective lib or *-dev packages on iOS.
    • Unsure about OSX

Where has this been tested?

Operating System:

Platform:

Target Platform:

Toolchain Version:

SDK Version:

Copy link
Member

@leptos-null leptos-null left a comment

Choose a reason for hiding this comment

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

I think this is great. I would like to verify that there are no major projects that use these headers.

I also think we should provide very clear instructions on how to switch to using the *-lib packages on each platform for developers that are impacted.

@L1ghtmann
Copy link
Member Author

Agree regarding the usage check. Re additional instructions, where do you think they should be placed? As examples in the added code rules statement?

@leptos-null
Copy link
Member

where do you think [additional instructions] should be placed?

Maybe a wiki page on this repo.

I'm thinking this because it's for users (i.e. theos users) less than developers (i.e. theos contributors).


Draft

Theos no longer provides headers for some libraries that provide headers through their own releases. For example, OpenSSL provides its headers in the openssl-dev package. This more easily allows developers to use the headers matching their deployment target.

To use headers from these libraries with Theos, follow these steps:

  1. Install the associated dev package on the device you're targeting (often this is your iOS device)
  2. Copy the header files the package installs to $THEOS/includes/. Usually packages install the headers to /usr/local/include/. For example, the headers may be located at /usr/local/include/openssl/. Copy this to your Theos development environment at $THEOS/include/openssl/.

@L1ghtmann
Copy link
Member Author

Ooo, hadn't thought of the wiki. Nice, thanks! Also, just to clarify, the headers don't need to be moved anywhere once installed since they're installed to a system include path.

@L1ghtmann
Copy link
Member Author

New wiki page has been created! Guess the last item would be confirming the existing headers aren't widely used. Not sure how exactly to best approach verifying that, so ideas are welcome

@leptos-null
Copy link
Member

Thanks for the wiki page!
I did a search on GitHub for https://github.com/search?q=THEOS++path%3A**%2FMakefile+archive&type=code and found only two projects.

I'm comfortable moving forward with this.

@L1ghtmann
Copy link
Member Author

Great, thanks for checking! I can modify my project in the next update and have posted a pr to Trollstore with the necessary change.

@leptos-null
Copy link
Member

Forgot to add the search for OpenSSL https://github.com/search?q=THEOS++path%3A**%2FMakefile+openssl&type=code

which had 2 results, both of which use a custom include path for OpenSSL, so this is a step forward.

@L1ghtmann L1ghtmann requested a review from kirb January 1, 2024 17:11
@L1ghtmann L1ghtmann merged commit 3f40f10 into theos:master Jan 27, 2024
@L1ghtmann L1ghtmann deleted the common branch January 27, 2024 04:42
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.

2 participants