search

PeterPawn / decoder

"secrets" decoding for FRITZ!OS devices
GNU General Public License v2.0
78 stars 18 forks source link

Some improvements #16

Closed besser82 closed 6 years ago

besser82 commented 6 years ago

As described in the commit messages.

besser82 commented 6 years ago

Rebased on recent origin/master branch.

besser82 commented 6 years ago

Added a commit to optionally install an unstripped binary.

besser82 commented 6 years ago

Now the change about the {C,LD}FLAGS is less intrusive.

besser82 commented 6 years ago

Yay! Thank you! =) I'll write you an email later during today.

besser82 commented 6 years ago

Well, gcc 4.7.x series (RHEL6) complains about the doubled typedef…

PeterPawn commented 6 years ago

OK, I'll stop now with immediate reactions ... looks like your changes aren't complete yet and it seems better to wait for your message.

er13 commented 6 years ago

Just wanted to point out the following Freetz patches to decoder's Makefile (those from 010 to 040).

PeterPawn commented 6 years ago

I wrote @besser82 an e-mail meanwhile and am awaiting the answer. I'll keep Freetz and the patches for "decrypt-fritzos-cfg" in mind (and I'll try to keep them as stable as possible) - it's one of the aspects I meant above in my comment.

er13 commented 6 years ago

@6ca3609: by appending to (instead of prepending) user/system supplied flags one makes sure that some flags are never get overwritten. I.e. if the desired behavior is "the user is allowed to set some flags specific for his system/configuration but not those important for some package feature to properly work" then appending is the way to go.

PeterPawn commented 6 years ago

It was my own mistake, that I commented all changes together as "approved" - the GitHub GUI led me to the wrong point, where I thought, the comment would apply only to a single commit (c5bc03e).

PeterPawn commented 6 years ago

I'm unable to reproduce eb28a71 with the (or better my) proposed settings for compiler warnings - here I need to know, which compiler version and which settings were used to produce a warning, if a "switch" statement "falls through its cases".

c5bc03e will be applied with other changes.

Instead of b7daa99 I'd prefer an additional make target 'install-nostrip' - imho it's easier to use than one more variable (tab completion, etc.).

The other changes to 'Makefile' (6ca3609 is the 2nd version of the proposal) need more explanation and discussion (as mentioned in my e-mail).

PeterPawn commented 6 years ago

No e-mail from @besser82 was received yet (even if it was announced) and there was no answer on my own e-mail, too.

To bring this request to a conclusion, I've added my own changes meanwhile to fix those parts, where I can see a reason to do anything.

The changes to CFLAGS and LDFLAGS settings would do the opposite of my own intention. I want to overrule any predefined settings in these variables in a manner, that after merging, my own preferred settings (at least for the specified options) will be used.

Using override CFLAGS += ... is equivalent to override CFLAGS := $(CFLAGS) ..., while the proposed changes would reverse this order.

https://www.gnu.org/software/make/manual/html_node/Override-Directive.html

So I'll close this PR here ... don't hesitate to start a discussion, if you think, I should overthink it again.