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

Addressing trailofbits#23, svcli: Support extracting the SignedData blobs from a PE #104

Closed
wants to merge 0 commits into from

Conversation

hugmyndakassi
Copy link
Contributor

As per your comment in #23 so it's more convenient to review

GNUmakefile Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I know GNUmakefile is technically more correct, but I'd prefer to keep it as Makefile for now -- IME the former is less common and causes more confusion for people who are less familiar with build systems and are just visually scanning the repo to find the makefile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning here was more that the name GNUmakefile will only be picked up by GNU make, whereas Makefile will be picked up by dozens of flavors of Make. And since GNU-make-isms are being used, it made sense to me. But your repo, your rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW: I have now created a floating branch for myself, with a GNUmakefile that includes your Makefile to keep things DRY. I don't like how complicated CMake makes things and the comments in at least one of the CMakeLists.txt from trailofbits speaks volumes. Guess I'm not alone after all.

"or use - for stdout)\n";
return 0;
}
return realmain(cmdl, extract);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than breaking this into realmain, can we keep this all as one function? The realmain name isn't super descriptive + passing in both a argh::parser and a partially extracted result (extract) makes it look like a leaky abstraction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite clear what it is you want. Disposing of realmain I got. But the remainder would naturally disappear then, no?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- sorry for the confusion, the second part was just me thinking out loud about why I'd prefer just one function here 🙂

@hugmyndakassi
Copy link
Contributor Author

Sorry, I didn't close this intentionally. It was caused by the fact that I had initially meant for you to pull from my master branch, but now I created a new branch instead. Will create a corresponding new PR shortly with the items you mentioned addressed. Hope they'll address all concerns you have.

@woodruffw
Copy link
Member

No problem, just give me a ping when the new one's open.

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