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

clean up code: GCC warnings; add black to travis; fix linux build errors #51

Merged
merged 25 commits into from Feb 13, 2019
Merged

clean up code: GCC warnings; add black to travis; fix linux build errors #51

merged 25 commits into from Feb 13, 2019

Conversation

yparitcher
Copy link
Contributor

fix some build errors on linux
clean up the build so only the appropriate parts are made for each build.
fixes all GCC warnings

@yparitcher yparitcher mentioned this pull request Jan 6, 2019
@nickray
Copy link
Member

nickray commented Jan 7, 2019

Are you sure about that GCC ARM ppa? Thanks a lot for looning into the Travis situation!

@yparitcher
Copy link
Contributor Author

that is arm's official ppa it should be the same as the compilers directly from their website.
i force pushed one commit so Travis is not testing this pull request any more, so i cant be sure if it passes

@nickray
Copy link
Member

nickray commented Jan 7, 2019

Do you have a Solo Hacker? If so could you test the generated builds for each of

tools/solotool.py program targets/stm32l432/all.hex --use-dfu --detach
tools/solotool.py program targets/stm32l432/solo.hex

via some U2F test page?

The reason I'm pushing this is that I am on Debian so I can't test PPAs properly, but the outcome of #50 (comment) was that Debian's packages did not produce working HEX files while the ARM website downloads do (they both build).

@yparitcher
Copy link
Contributor Author

the builds work on my computer using the ARM website downloads, as i only just found out about the ppa.
ubuntu and debian default package have a bug because incompatible versions of libraries were used in building the package
i have not tested the ppa but i can try it a little later.
what i was not sure about was the Travis config being set up 100%, however the code builds and runs on my solo, as a result of the bug-fixing i did

@nickray
Copy link
Member

nickray commented Jan 7, 2019

the builds work on my computer using the ARM website downloads, as i only just found out about the ppa.

ubuntu and debian default package have a bug because incompatible versions of libraries were used in building the package

nice!

i have not tested the ppa but i can try it a little later.

thank!

what i was not sure about was the Travis config being set up 100%, however the code builds and runs on my solo, as a result of the bug-fixing i did

Travis is a bit useless as is, so I'm happy to merge that part and we can iterate from there.

@nickray
Copy link
Member

nickray commented Jan 7, 2019

Would you be able to add something to Travis that runs black -S --check on the Python stuff that is not copy-pasted external code? You could build on the existing make env3 and make black targets, but note that we don't want to enforce using virtualenvs throughout the code (just helpfully make them available). If not, that's OK too :)

@nickray
Copy link
Member

nickray commented Jan 7, 2019

Oh, and can you point me to the Debian ARM package bug?

@yparitcher
Copy link
Contributor Author

bug reports

the venv stuff that i added is optional use like the prefix; just for the test i had set it so travis can use the python

the ppa toolchain builds the code, I will test flashing it later

@yparitcher
Copy link
Contributor Author

Rebased on master

@nickray
Copy link
Member

nickray commented Jan 8, 2019

Thanks for all these fixes so far, @conorpp will review the C changes.

@yparitcher yparitcher mentioned this pull request Jan 17, 2019
.travis.yml Outdated Show resolved Hide resolved
@@ -42,11 +42,15 @@ tinycbor/Makefile crypto/tiny-AES-c/aes.c:
.PHONY: cbor
cbor: $(LIBCBOR)

$(LIBCBOR): tinycbor/Makefile
$(LIBCBOR):
Copy link
Contributor

Choose a reason for hiding this comment

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

If you leave that dependency in, it should automatically check out the submodule if it's not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

however it will then only rebuild Cbor if the makefile (which does not happen) changes even if you change architectures, which breaks builds

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing architecture doesn't indicate a code change. I think if you're going to change architecture you just have to do a make clean first. If the current make clean doesn't clean tinycbor then that should be fixed, which would then allow this auto-checkout still work and an alternate architecture to build correctly after a make clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point will fix

Makefile Show resolved Hide resolved
@yparitcher
Copy link
Contributor Author

@pjz the makefile in general is overly complex, and not consistent and should probably be rewritten, however i tried to work around the existing structure. thanks for the review

.travis.yml Outdated Show resolved Hide resolved
@yparitcher
Copy link
Contributor Author

Travis build finaly fixed with stm32l432 code built and black run on the python

@cla-bot
Copy link

cla-bot bot commented Jan 21, 2019

We require contributors to sign our Copyright License Agreement, and we don't have @yparitcher on file. In order for us to review and merge your code, please visit https://solokeys.com/legal/contributors, or contact @nickray, @conorpp or @0x0ece for further information or help.

@yparitcher
Copy link
Contributor Author

@nickray @conorpp @0x0ece Please review #77 and hopefully relicence under GPLv2 thereby using a solid open source licence without needing CLAs and other circumventions to the GPLv3 that put everything in the same spot with added hassle & contributors work are not totally open source.
Thanks

This was referenced Jan 21, 2019
@cla-bot
Copy link

cla-bot bot commented Jan 22, 2019

We require contributors to sign our Copyright License Agreement, and we don't have @yparitcher on file. In order for us to review and merge your code, please visit https://solokeys.com/legal/contributors, or contact @nickray, @conorpp or @0x0ece for further information or help.

@yparitcher
Copy link
Contributor Author

Rebased on master

@yparitcher yparitcher changed the title clean up build: GCC warnings clean up code: GCC warnings; add black to travis; fix linux build errors Jan 22, 2019
@yparitcher
Copy link
Contributor Author

my commits in this pull request are licensed under the GPLv2 thereby resolving licensing issues.

@cla-bot
Copy link

cla-bot bot commented Feb 5, 2019

We require contributors to sign our Copyright License Agreement, and we don't have @yparitcher on file. In order for us to review and merge your code, please visit https://solokeys.com/legal/contributors, or contact @nickray, @conorpp or @0x0ece for further information or help.

Copy link
Member

@conorpp conorpp left a comment

Choose a reason for hiding this comment

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

I like getting rid of the warnings for sure, but I don't like adding

#if DEBUG_LEVEL > 0
...
#endif

for every unused variable. It's a bit messy, and I think it'd be better to leave/ignore the unused-related warnings.

@conorpp
Copy link
Member

conorpp commented Feb 12, 2019

Sorry for taking a long time to get around to this. Could you take a look at the conflicts? Also would it be okay to dual license under MIT+Apache2? Just updated the license for the repo.

@cla-bot
Copy link

cla-bot bot commented Feb 13, 2019

We require contributors to sign our Copyright License Agreement, and we don't have @yparitcher on file. In order for us to review and merge your code, please visit https://solokeys.com/legal/contributors, or contact @nickray, @conorpp or @0x0ece for further information or help.

@cla-bot
Copy link

cla-bot bot commented Feb 13, 2019

We require contributors to sign our Copyright License Agreement, and we don't have @yparitcher on file. In order for us to review and merge your code, please visit https://solokeys.com/legal/contributors, or contact @nickray, @conorpp or @0x0ece for further information or help.

@yparitcher
Copy link
Contributor Author

MIT+Apache2 is fine

@yparitcher
Copy link
Contributor Author

Please remove the cla-bot

@yparitcher
Copy link
Contributor Author

let me know if i need to change anything

@conorpp
Copy link
Member

conorpp commented Feb 13, 2019

Looks good, thanks!!

@conorpp conorpp merged commit e7e8382 into solokeys:master Feb 13, 2019
@yparitcher yparitcher deleted the clean_build branch February 13, 2019 02:02
nabijaczleweli pushed a commit to nabijaczleweli/solo that referenced this pull request Mar 31, 2021
Ensure compatibility with python-fido2 0.8 (fixes solokeys#49)
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.

None yet

4 participants