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

feat: Expose proguard remapper via c-abi #240

Merged
merged 9 commits into from
Jun 9, 2020
Merged

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented May 26, 2020

Exposes the new proguard::Mapper via cabi and py.

Keeps the old MappingView and workspace crate untouched, so this can be released as a minor version, while deprecating and removing the other stuff at some later point.

@Swatinem Swatinem requested a review from a team June 5, 2020 08:11
@@ -28,7 +28,7 @@ enum SymbolicErrorCode {
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_UNSUPPORTED_OBJECT = 2101,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_BREAKPAD_OBJECT = 2102,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_ELF_OBJECT = 2103,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_MACH_OOBJECT = 2104,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_MACH_O_OBJECT = 2104,
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if this will cause any problems?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering the same...

Copy link
Member Author

Choose a reason for hiding this comment

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

I manually reverted this. Although its an auto-generated file, so people will run into this again at some point.

Copy link
Member

Choose a reason for hiding this comment

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

This is indeed strange. It would be a breaking change in the C-ABI, and it will generate a different name for the matching error in the Python layer. I think you can fix this with some sort of configuration for cbindgen, although the new version is actually correct.

@Swatinem Swatinem marked this pull request as ready for review June 5, 2020 10:32
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

other than this mach-o thing i think this looks fine

@@ -28,7 +28,7 @@ enum SymbolicErrorCode {
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_UNSUPPORTED_OBJECT = 2101,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_BREAKPAD_OBJECT = 2102,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_ELF_OBJECT = 2103,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_MACH_OOBJECT = 2104,
SYMBOLIC_ERROR_CODE_OBJECT_ERROR_BAD_MACH_O_OBJECT = 2104,
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering the same...

cabi/Cargo.toml Show resolved Hide resolved
cabi/src/proguard.rs Outdated Show resolved Hide resolved
cabi/src/proguard.rs Outdated Show resolved Hide resolved
cabi/src/proguard.rs Outdated Show resolved Hide resolved
@jan-auer
Copy link
Member

jan-auer commented Jun 8, 2020

One more ask: What if we mark the old C-ABI functions and Python functions as deprecated? Once we do the next major bump, it's easier to find all the places to remove.

@Swatinem
Copy link
Member Author

Swatinem commented Jun 8, 2020

👍 I was able to shim the old functions quite easily, and also copied the SelfCell code over to the cabi.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Thank you! I'll run a release once this is merged.

@jan-auer jan-auer merged commit feeec31 into master Jun 9, 2020
@jan-auer jan-auer deleted the feat/proguard-remapping branch June 9, 2020 09:07
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