Skip to content

wolfSSL Alpha examples#6197

Closed
kaleb-himes wants to merge 2 commits intoRIOT-OS:masterfrom
kaleb-himes:wolfSSL-ALPHA
Closed

wolfSSL Alpha examples#6197
kaleb-himes wants to merge 2 commits intoRIOT-OS:masterfrom
kaleb-himes:wolfSSL-ALPHA

Conversation

@kaleb-himes
Copy link
Contributor

These examples have only been tested successfully on MAC OS X using the following toolchain:

$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/include/c++/4.2.1
Apple LLVM version 8.0.0 (clang-800.0.42.1)
Target: x86_64-apple-darwin16.1.0
Thread model: posix

This is strictly an alpha release to allow the RIOT-OS community and wolfSSL community to begin collaborating! There may be portability issues if building with another gcc version. Please report all issues and direct all development related questions to support@wolfssl.com

For all other questions please contact info@wolfssl.com

We look forward to working with RIOT-OS community to provide an IoT friendly ssl/tls and/or cryptographic solution!

wolfSSL can be compiled for cryptography only by adding WOLFCRYPT_ONLY to CFLAGS

directory rework

note directory change in help printout, update .gitignore

Update help print outs in scripts

update .gitignore for --no-cleanup testing
@kaleb-himes kaleb-himes reopened this Dec 10, 2016
@LudwigKnuepfer LudwigKnuepfer added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Dec 10, 2016
@LudwigKnuepfer
Copy link
Member

Some comments:

  • It is required per our development procedures that discussion and takes place on github (as opposed to email as your PR description says).
  • I am kind of surprised that there is neither a package, (compare documentation) nor code or at least a Makefile in the examples.
  • Tests should go into /tests.

@LudwigKnuepfer LudwigKnuepfer added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: pkg Area: External package ports labels Dec 10, 2016
@kaleb-himes
Copy link
Contributor Author

kaleb-himes commented Dec 10, 2016

Hi LudwigKnuepfer,

  1. I can change the names from Makefile.testname to just Makefile if that is more helpful.

  2. Given that we had pre-existing tests that built fine it seemed unnecessary to write entirely new main.c, simpler to just copy them over from the wolfSSL package.

  3. I will happily move to the tests/ directory if that is preferred!

@kYc0o
Copy link
Contributor

kYc0o commented Dec 14, 2016

As Ludwig suggests, this kind of new features should be located on the pkg/ folder, where you add some Makefiles which will download automatically the new code, add some patches if necessary and compile it (either using RIOT's compiling rules or upstream, you can get examples of this in any pkg_* prefixed test).

The test should be located under tests/ on which the Makefile should use the associated pkg (look for instance to tests prefixed by pkg_* on the tests folder) and eventually a main program performing the tests.

As far as I can see, everything is based on scripts which will do the job, which is not very common in RIOT, but neither is forbidden nor non-recommended. I'd like to have the opinion of @OlegHahm or @kaspar030 on this.

Whenever possible, we prefer Markdown readme, but that's just a detail.

Commits should be also prefixed by the place where they are modifying/adding code, for instance:

tests/wolfSSL: adding wolfSSL package tests

Any commit addressing reviewers comments should not be squashed until the reviewer ACK your PR. Those commits can have any name and description.

I'll test it asap and give feedback, but this PR needs to reorganise the way wolfSSL is added to RIOT before getting merged.

Thanks for your contribution!

@kYc0o kYc0o self-requested a review December 14, 2016 16:25
@kYc0o kYc0o self-assigned this Dec 14, 2016
@OlegHahm
Copy link
Member

As far as I can see, everything is based on scripts which will do the job, which is not very common in RIOT, but neither is forbidden nor non-recommended. I'd like to have the opinion of @OlegHahm or @kaspar030 on this.

While this procedure is not explicitly "forbidden", I would vote against introducing it this way. @kaleb-himes, is there any reason against introducing wolfSSL as a package? IMO this is way more convenient for an application developer. With the proposed method in this PR an application developer who wants to use wolfSSL on top of RIOT would need to locate and copy over the script from the test folder instead of simply adding USEPKG += wolfssl to the application's Makefile.

@kaleb-himes
Copy link
Contributor Author

Hi @OlegHahm and @kYc0o

Excellent suggestions! I will research your documentation on how to create a package in this manner. I agree that would be far more convenient from a developer standpoint to be able to add USEPKG += wolfssl to the Makefile.

Thank you so much for all the feedback and helpful commentary thus far!

@OlegHahm
Copy link
Member

Great!

Let us know if you have problems or questions regarding the documentation and creating a package. We will happily help you and may have a chance to improve the docs.

@kaleb-himes
Copy link
Contributor Author

Thank you, will do!

@emmanuelsearch
Copy link
Member

@kaleb-himes what's the latest on this?
FYI we have release planned at the end of the month ;)

@kaleb-himes
Copy link
Contributor Author

kaleb-himes commented Jan 24, 2017

Hi @emmanuelsearch

We have been extremely busy since the 1st of the year and it has severely impacted my progress on this. No rush before your release, it may be some time before I am free to look into generating the requested "package". We have RSA 2017 quickly approaching and many paid projects in the queue!

Once we get some breathing room again this will be one of the first things on our to-do list. If at all possible wolfSSL is happy to start with what we already submitted, gather user feedback, and put out a package, per your request, in a few months or at the earliest convenience.

Warmest Regards,

Kaleb

@miri64
Copy link
Member

miri64 commented Apr 29, 2017

People are asking for the progress of this PR ;-) https://twitter.com/wolfSSL/status/857376741706186752

@miri64
Copy link
Member

miri64 commented May 24, 2017

Ping?

@aabadie aabadie modified the milestone: Release 2017.07 Jun 26, 2017
@kYc0o
Copy link
Contributor

kYc0o commented Jul 7, 2017

Hi @kaleb-himes! Do you have some progress on this PR?


# wolfSSL security
tests/security/wolfssl
tests/security/*/*.c
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this should be here... If while performing your test you download the package as other tests do, the sources are automatically ignored.

CFLAGS += -DBENCH_EMBEDDED

#ignore warnings on empty translation units due to wolfSSL configurability
WERROR = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be used in a test.

@kaleb-himes
Copy link
Contributor Author

Closing this pull request based on feedback. wolfSSL has instead created a "PKG" solution and opened a new PR. See new PR here: #7348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: pkg Area: External package ports State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants