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

chore: update lief #23

Merged
merged 1 commit into from
Sep 20, 2022
Merged

chore: update lief #23

merged 1 commit into from
Sep 20, 2022

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Sep 20, 2022

This change updates lief to lief-project/LIEF@b183666.

This contains lief-project/LIEF#780, which is a requirement for #8.

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

This change updates lief to
lief-project/LIEF@b183666.

This contains lief-project/LIEF#780, which is
a requirement for #8.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen changed the title Update lief chore: update lief Sep 20, 2022
@RaisinTen RaisinTen merged commit d7c54a8 into main Sep 20, 2022
@RaisinTen RaisinTen deleted the update-lief branch September 20, 2022 14:27
@dsanders11
Copy link
Contributor

FWIW, I think in the future we should be cautious about updating LIEF when there's a large PR like #8 in-progress, since it may destabilize the PR and require more work to fix it back up, especially since the LIEF version being updated to isn't a stable release.

@RaisinTen
Copy link
Contributor Author

We should have more tests to make sure that such a change isn't breaking any relevant component of postject. I don't think there's any other way to be sure otherwise. Also, this change was supposed to make the review process easier for that PR because it reduces a patch.

@dsanders11
Copy link
Contributor

Also, this change was supposed to make the review process easier for that PR because it reduces a patch.

It reduces a patch, but brings in N new commits from upstream, which is a mixed bag, and is the point I was making.

@RaisinTen
Copy link
Contributor Author

Yes but the new commits shouldn't be problematic for us if it passes the test suite which is why I'm emphasizing on building a test suite ASAP that has tests for everything we care about.

@dsanders11
Copy link
Contributor

Yes but the new commits shouldn't be problematic for us

They can always be problematic for us in that they might break the build and require more work - even if it works on main. That's the reason that patch exists in the first place, those problems only appeared on the WebAssembly branch. So even if this PR didn't run into issues on main, it could still create more work on the WebAssembly branch due to those new commits, creating more work there.

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.

3 participants