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

CTAP library move #602

Merged
merged 3 commits into from
Mar 7, 2023
Merged

CTAP library move #602

merged 3 commits into from
Mar 7, 2023

Conversation

kaczmarczyck
Copy link
Collaborator

Moves all source files into its own library

I removed the Cargo.lock files from all libraries. As a consequence of this decision and the CTAP move, I changed the workflows and run_desktop_tests.sh a bit to reflect the new reality better.

What might be interesting is that some tests and fuzzing now run on nightly. This is because some of our dependency and cargo fuzz don't work with Tock's older nightly, if we don't properly pin to older versions. I'd say we care that the OpenSK binary builds and works with the pinned toolchain, and our libraries build and test correctly with the latest toolchain. This might cause sudden and unrelated problems, but I think we want to know.

Also I hope that the local test script runs on a fresh computer that doesn't have my Rust setup, with the new usage of 2 compiler versions. Otherwise, this PR contains no actual code changes.

@coveralls
Copy link

coveralls commented Mar 7, 2023

Coverage Status

Coverage: 96.058% (+8.5%) from 87.587% when pulling 8de7b79 on kaczmarczyck:full-move into 03031e6 on google:develop.

@kaczmarczyck kaczmarczyck requested a review from ia0 March 7, 2023 08:45
@kaczmarczyck
Copy link
Collaborator Author

I'll look into Coveralls, but you can probably review everything else.

Copy link
Member

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Now that we plan to have proper published libraries, I think we should change a few things:

  1. We should have one workflow per published library.
  2. We should specify the MSRV (minimal supported rust version) in the Cargo.toml under package.rust-version.
  3. The workflow of each library should have a strategy matrix to check build with the main toolchains: MSRV, latest stable, latest nightly.

EDIT: This can/should probably be in a follow-up PR. Just mentioning it here because those files were touched.

.github/workflows/cargo_fmt.yml Outdated Show resolved Hide resolved
.github/workflows/cargo_fuzz.yml Outdated Show resolved Hide resolved
libraries/opensk/fuzz/Cargo.lock Outdated Show resolved Hide resolved
run_desktop_tests.sh Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@kaczmarczyck kaczmarczyck force-pushed the full-move branch 2 times, most recently from e441ed7 to 96c7a44 Compare March 7, 2023 14:04
ia0
ia0 previously approved these changes Mar 7, 2023
@kaczmarczyck kaczmarczyck merged commit ca65902 into google:develop Mar 7, 2023
@kaczmarczyck kaczmarczyck deleted the full-move branch March 7, 2023 14:56
DavidKorczynski pushed a commit to google/oss-fuzz that referenced this pull request Mar 8, 2023
OpenSK moved its CTAP code into a library, and all fuzzing has moved
with it. As an advantage, we are now independent of other dependencies
that restricted the compiler versions we could use. Therefore, we are
now fuzzing on the latest nightly, hopefully fixing some issues that
came with the compiler restriction before.

The corresponding PR on our GitHub is
google/OpenSK#602
@kaczmarczyck kaczmarczyck mentioned this pull request Mar 9, 2023
2 tasks
eamonnmcmanus pushed a commit to eamonnmcmanus/oss-fuzz that referenced this pull request Mar 15, 2023
OpenSK moved its CTAP code into a library, and all fuzzing has moved
with it. As an advantage, we are now independent of other dependencies
that restricted the compiler versions we could use. Therefore, we are
now fuzzing on the latest nightly, hopefully fixing some issues that
came with the compiler restriction before.

The corresponding PR on our GitHub is
google/OpenSK#602
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.

3 participants