Skip to content

pkg/cryptoauthlib: cleanup build system integration#14268

Merged
leandrolanzieri merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/pkg/cryptoauthlib_cleanup
Jun 19, 2020
Merged

pkg/cryptoauthlib: cleanup build system integration#14268
leandrolanzieri merged 2 commits intoRIOT-OS:masterfrom
aabadie:pr/pkg/cryptoauthlib_cleanup

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Jun 11, 2020

Contribution description

This PR cleans up how the cryptoauthlib package is built:

  • it removes the need to move around source files within the package source: for this we can directly use RIOT's build system module functionality.
  • it modifies the way CMake is used to perform an out-of-source build . This is a recommended usage with CMake: all files generated by the configuration and build are located outside the original source code.

There's also a minor change: the BUILD_TESTS doesn't exists in the variables defined by the project. The actual build generates the following warning:

CMake Warning:
  Manually-specified variables were not used by the project:

    BUILD_TESTS

This PR simply removes it to drop the warning.

Testing procedure

  • A green Murdock
  • The test/pkg_cryptoauthlib_internal-tests still builds and works fine

Issues/PRs references

None

@aabadie aabadie added Area: build system Area: Build system Area: pkg Area: External package ports Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 11, 2020
@aabadie aabadie requested a review from PeterKietzmann as a code owner June 11, 2020 15:36
@aabadie aabadie force-pushed the pr/pkg/cryptoauthlib_cleanup branch from ea96600 to ab26a52 Compare June 11, 2020 18:34
@aabadie aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 12, 2020
@Einhornhool
Copy link
Copy Markdown
Contributor

Einhornhool commented Jun 12, 2020

I will review this next week when I have access to a CryptoAuth extension board.

@aabadie aabadie force-pushed the pr/pkg/cryptoauthlib_cleanup branch 5 times, most recently from aeccc29 to f948689 Compare June 13, 2020 13:54
@aabadie aabadie force-pushed the pr/pkg/cryptoauthlib_cleanup branch from f948689 to ff37e8e Compare June 13, 2020 19:06
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 15, 2020

I will review this next week when I have access to a CryptoAuth extension board.

Thanks!

@aabadie aabadie mentioned this pull request Jun 15, 2020
Merged
Einhornhool
Einhornhool approved these changes Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@Einhornhool Einhornhool left a comment

Choose a reason for hiding this comment

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

I ran this on a saml21 with the cryptoauth explained extension board and it works fine.

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 19, 2020

Thanks for testing @Einhornhool. Can someone ACK ? @leandrolanzieri or @fjmolinas ?

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Jun 19, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.07 milestone Jun 19, 2020
leandrolanzieri
leandrolanzieri approved these changes Jun 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Changes look good. Makefile is cleaner. @Einhornhool tested on hardware and CI is green -> ACK

@leandrolanzieri leandrolanzieri merged commit e7dcc28 into RIOT-OS:master Jun 19, 2020
@aabadie aabadie deleted the pr/pkg/cryptoauthlib_cleanup branch June 24, 2020 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

+1 more reviewer

@Einhornhool Einhornhool Einhornhool approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2020.07

Development

Successfully merging this pull request may close these issues.

3 participants

@aabadie @Einhornhool @leandrolanzieri