Add rustfmt and Github CI by G8XSU · Pull Request #2 · lightningdevkit/vss-client

G8XSU · 2023-04-26T22:04:14Z

No description provided.

tnull

Seems this is based on #1?

Can we maybe instead rebase #1 on this to avoid the additional cargo fmt commit that is somewhat out-of-place here?

tnull · 2023-05-02T09:46:50Z

.github/workflows/build.yml

+  build:
+    strategy:
+      matrix:
+        toolchain: [ stable, beta ]


What MSRV will the VSS client support? Can we start enforcing it here in CI from the getgo?

I haven't gotten a chance to look into that, when i do, will start enforcing it.

Great! Mind creating a tracking issue for it?

Created : #4

G8XSU · 2023-05-02T21:37:05Z

Regarding basing everything on #2, i don't think it matters much. will have to reorder and rebase every PR.
Once #1 is merged, rebased changes should be easy.

Currently commits are just in the order in which work was done.

tnull · 2023-05-03T09:24:44Z

Regarding basing everything on #2, i don't think it matters much. will have to reorder and rebase every PR. Once #1 is merged, rebased changes should be easy.

Currently commits are just in the order in which work was done.

Well, it would avoid the additional cargo fmt commit. Also I don't believe CI is actually running currently, so it would be nice to first land the new CI scripts and then have CI run on the actual code changes.

.github/workflows/build.yml

tnull · 2023-05-04T07:08:00Z

.github/workflows/build.yml

+  build:
+    strategy:
+      matrix:
+        toolchain: [ stable, beta ]


Great! Mind creating a tracking issue for it?

tnull · 2023-05-04T07:11:28Z

.github/workflows/build.yml

+      - name: Checkout source code
+        uses: actions/checkout@v2
+      - name: Install Protobuf compiler (protoc)
+        uses: arduino/setup-protoc@v1


Any reason we can't just simply run sudo apt-get update && sudo apt-get -y install protobuf-compiler?

i think that one is platform agnostic, but i think we could replace with simpler command above.

tnull

LGTM, mod one comment.

tnull · 2023-05-30T10:07:14Z

.github/workflows/build.yml

+          profile: minimal
+      - name: Build on Rust ${{ matrix.toolchain }}
+        run: cargo build --verbose --color always
+      - name: Check formatting


Probably also want to add another step enforcing buildable docs for the public API, i.e.,

cargo check --release cargo doc --release

In combination with adding

#![deny(missing_docs)] #![deny(rustdoc::broken_intra_doc_links)] #![deny(rustdoc::private_intra_doc_links)]

To your lib.rs file.

👍
For now adding only broken_intra_doc_links and private_intra_doc_links
Some of the auto-generated code doesn't comply with all the checks.

Mh, but isn't that simply a function of the docs missing in https://github.com/lightningdevkit/vss-server/blob/main/app/src/main/proto/vss.proto? Should they be added there rather than omitting the check here?

"for now",
will add it eventually. I can add at warn level for now.
(already have some changes in pipeline for proto file)

tnull · 2023-07-10T07:26:08Z

.github/workflows/build.yml

+          profile: minimal
+      - name: Build on Rust ${{ matrix.toolchain }}
+        run: cargo build --verbose --color always
+      - name: Check formatting


tnull · 2023-07-10T07:31:28Z

vss-accessor/tests/generate_protos.rs

+// cargo build && OUT_DIR=../target/tmp/ cargo test generate_protos -- --exact
+// ```
+#[test]
+#[ignore]


Not quite sure why we have this test case here? Seems this should be part of (a) build script rather than live in the tests folder? Also we probably don't want to use ignore and have the user uncomment it, but rather use a cfg(..) for this. This also breaks a simple cargo test for me:

> cargo test Compiling prost v0.11.9 Compiling regex-syntax v0.7.1 Compiling vss-accessor v0.1.0 (/Users/ero/workspace/vss-rust-client/vss-accessor) Compiling reqwest v0.11.17 Compiling prost-types v0.11.9 Compiling regex v1.8.1 Compiling prost-build v0.11.9 error: environment variable `OUT_DIR` not defined --> vss-accessor/tests/generate_protos.rs:22:19 | 22 | fs::copy(concat!(env!("OUT_DIR"), "/org.vss.rs"), "src/generated-src/org.vss.rs").unwrap(); | ^^^^^^^^^^^^^^^ | = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info) error: could not compile `vss-accessor` due to previous error warning: build failed, waiting for other jobs to finish... exit 101

i earlier had this in build.rs, on feedback had moved it to test which runs optionally.
but i think build.rs with conditional build is the right approach.

.github/workflows/build.yml

tnull

Generally LGTM!

Only one formatting nit and one question. Should be good to go after that though.

vss-accessor/src/lib.rs

tnull · 2023-07-11T07:09:15Z

vss-accessor/src/generated-src/org.vss.rs

Why are we checking in the generated sources if we're downloading and building them from a specific server commit? Just to have clear versioning of the proto file?

yes and people felt more comfortable with static generated code over generated code for every build.

i thought we discussed this already hence this PR was branched out and created.

Alright, makes sense.

tnull

LGTM.

G8XSU force-pushed the ci-fmt branch from 01ab1e4 to f66c748 Compare April 26, 2023 22:05

G8XSU marked this pull request as ready for review April 26, 2023 22:07

G8XSU force-pushed the ci-fmt branch from 6d67dda to 993038e Compare April 26, 2023 22:45

G8XSU mentioned this pull request Apr 27, 2023

Add Unit tests for VssAccessor #3

Merged

tnull reviewed May 2, 2023

View reviewed changes

tnull reviewed May 3, 2023

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

G8XSU force-pushed the ci-fmt branch 6 times, most recently from dc423af to 0715f27 Compare May 3, 2023 23:30

G8XSU mentioned this pull request May 4, 2023

Add VSS Http thin client implementation for get/put/listKeyVersions api's #1

Merged

tnull reviewed May 4, 2023

View reviewed changes

G8XSU force-pushed the ci-fmt branch from 0715f27 to 73ce6a3 Compare May 4, 2023 22:14

G8XSU mentioned this pull request May 10, 2023

VSS Tracking Issue lightningdevkit/vss-server#9

Closed

31 tasks

tnull reviewed May 30, 2023

View reviewed changes

G8XSU force-pushed the ci-fmt branch from 73ce6a3 to acb55bc Compare July 5, 2023 22:31

tnull reviewed Jul 10, 2023

View reviewed changes

.github/workflows/build.yml Outdated Show resolved Hide resolved

G8XSU force-pushed the ci-fmt branch from acb55bc to 1004470 Compare July 11, 2023 00:21

tnull reviewed Jul 11, 2023

View reviewed changes

Add Vss thin client protobuf generation & build/CI setup

143d762

G8XSU force-pushed the ci-fmt branch from 1004470 to 143d762 Compare July 11, 2023 17:01

G8XSU requested a review from tnull July 11, 2023 17:01

tnull approved these changes Jul 11, 2023

View reviewed changes

G8XSU merged commit fe4f654 into lightningdevkit:main Jul 12, 2023

Conversation

G8XSU commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU commented May 2, 2023

Uh oh!

tnull commented May 3, 2023

Uh oh!

Uh oh!

tnull May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

G8XSU commented Apr 26, 2023 •

edited

Loading

tnull May 4, 2023 •

edited

Loading

tnull May 4, 2023 •

edited

Loading