search

nodejs / postject

Easily inject arbitrary read-only resources into executable formats (Mach-O, PE, ELF) and use it at runtime.
Other
187 stars 14 forks source link

chore: remove unnecessary files from `lief` #33

Closed RaisinTen closed 2 years ago

RaisinTen commented 2 years ago

The mask removes the files that we won't be needing in this project.

Signed-off-by: Darshan Sen raisinten@gmail.com

dsanders11 commented 2 years ago

I'm not necessarily opposed to this change, but what's the main motivation here? Disk space savings? Is it anything significant?

I think there's a slight downside to this when it comes to patches since it might require modifying patches to remove changes to masked out paths, so I want to consider the benefits against that downside before approving. If it only saves ~25 MB of disk space I'd be inclined to leave it as is so that patching remains trivial, and save developer time as a result.

RaisinTen commented 2 years ago

@dsanders11 new contributors might visit the vendor/lief folder to get a rough understanding of how things are interlinked. Keeping files that are not really related to this project might confuse them into thinking that those files actually get used, so this change should fix that issue to an extent.

Space is also a reason, yes - both because it saves space on the file system and because it boosts confidence among new contributors. The smaller the project the more you think that there are chances that you can actually wrap your head around the full thing.

when it comes to patches since it might require modifying patches to remove changes to masked out paths

It feels unlikely that we'll have to maintain a number of backported patches of this nature because I think that we're pretty good at staying on the latest version of lief?

dsanders11 commented 2 years ago

@dsanders11 new contributors might visit the vendor/lief folder to get a rough understanding of how things are interlinked. Keeping files that are not really related to this project might confuse them into thinking that those files actually get used, so this change should fix that issue to an extent.

I understand the motivation argument here, but I don't really agree with the assessment. There's still a large amount of code in vendor/lief which is unused (ART, DEX, OAT, VDEX, etc) by Postject, so this PR only removes some of the unused code, while also removing things like docs, examples, and tests which are useful for understanding how the remaining code works. For example, there are useful C++ examples in vendor/lief/examples/cpp. I know the exact paths masked could be tweaked, but the larger point is I think slicing out some parts of vendor/lief doesn't really accomplish the stated motivation and might be counter-productive to those ends. The Python API code, while unused in Postject, is conceptually quite similar to the Emscripten wrapper code we do write, so it can be useful to look at the existing Python API for comparison.

It feels unlikely that we'll have to maintain a number of backported patches of this nature because I think that we're pretty good at staying on the latest version of lief?

Unless we feel like waiting for it to land upstream before landing it in Postject every time, there's plenty of potential we will have patches going forward. At the moment LIEF is good at merging PRs quickly, but that could change at any time, it's just one maintainer, and there hasn't been a release since April 2022. We may at some point find we can't take HEAD from upstream due to breakage and need to add a patch reverting some change, for example.

RaisinTen commented 2 years ago

Hmm, good point. I'm okay with closing this then. :+1: