search

drogue-iot / drogue-cloud

Cloud Native IoT
https://drogue.io
Apache License 2.0
114 stars 30 forks source link

Limit `actix-web` dep to `service-api` only #219

Closed jcrossley3 closed 2 years ago

jcrossley3 commented 2 years ago

The primary motivation for this PR is to declare the actix-web version that all the drogue-cloud crates depend on in a single place. In this PR, that place is service-api/Cargo.toml.

Initially, this is mostly re-exporting actix-web abstractions from the service-api crate, but it can evolve to be smarter and more suited to how the drogue web apps use actix.

The biggest challenge was the actix-web::main proc macro, since it generates code assuming actix-web is declared as a dep. Re-exporting it from service-api required implementing our own proc-macro=true crate: macros. Ugh.

The new macros crate could possibly justify its existence by moving the service-common/src/app.rs stuff over to it, but that's for another PR.

jcrossley3 commented 2 years ago

/test

drogue-bot commented 2 years ago

Aye, aye, captain! … Triggered System Test worklow!

drogue-bot commented 2 years ago
Total Passed Failed Ignored Filtered Duration
✅ 147 147 0 0 0 30m 51s

Git: https://github.com/drogue-iot/drogue-cloud @ refs/heads/de-actix-web-ify

Commit: 2a31b42bb0c02c27a6dc5c6077a0c7e54d87d320
Author: Jim Crossley <jim@crossleys.org>
Date: Tue, 25 Jan 2022 22:39:56 +0500

    PoC limiting actix-web dep to service-common only

    Initially, this is mostly re-exporting actix-web abstractions from
    service-common, but it can evolve to be smarter and more suited to how
    the drogue web apps use actix.

    The biggest challenge was the actix-web::main proc macro, since it
    generates code assuming actix-web is declared as a dep. Re-exporting
    it from service-common required implementing our own proc-macro=true
    crate: macros. Ugh.

    The new macros crate could possibly justify its existence by moving
    the `service-common/src/app.rs` stuff over to it, but that's for
    later.

    Before infecting the other workspaces with this, let's make sure CI
    can stomach this.

Job: https://github.com/drogue-iot/drogue-cloud-testing/actions/runs/1747324196

Report: https://drogue-iot.github.io/drogue-cloud-testing/test-report/2022/01/25/test-run-1747324196.html

jcrossley3 commented 2 years ago

/test

drogue-bot commented 2 years ago

Aye, aye, captain! … Triggered System Test worklow!

drogue-bot commented 2 years ago
Total Passed Failed Ignored Filtered Duration
✅ 147 147 0 0 0 31m 33s

Git: https://github.com/drogue-iot/drogue-cloud @ refs/heads/de-actix-web-ify

Commit: 3ab8ed446fbf91da1213238ae6e2b4b68a0aadac
Author: Jim Crossley <jim@crossleys.org>
Date: Wed, 26 Jan 2022 05:57:11 +0500

    Remove actix-web dep from authentication-service

Job: https://github.com/drogue-iot/drogue-cloud-testing/actions/runs/1749097046

Report: https://drogue-iot.github.io/drogue-cloud-testing/test-report/2022/01/26/test-run-1749097046.html

ctron commented 2 years ago

That is an interesting approach. I really like it!

jcrossley3 commented 2 years ago

/test

drogue-bot commented 2 years ago

Aye, aye, captain! … Triggered System Test worklow!

drogue-bot commented 2 years ago
Total Passed Failed Ignored Filtered Duration
✅ 147 147 0 0 0 31m 13s

Git: https://github.com/drogue-iot/drogue-cloud @ refs/heads/de-actix-web-ify

Commit: 81df6c256a618f6420341b9aa0af305a6d13d633
Author: Jim Crossley <jim@crossleys.org>
Date: Thu, 27 Jan 2022 23:13:11 +0500

    Featurize the actix deps in service-api

    Enabled by default, because all but the console-frontend can build the
    actix stuff. WASM has trouble with sockets, understandably.

Job: https://github.com/drogue-iot/drogue-cloud-testing/actions/runs/1758046394

Report: https://drogue-iot.github.io/drogue-cloud-testing/test-report/2022/01/27/test-run-1758046394.html

jcrossley3 commented 2 years ago

/test

drogue-bot commented 2 years ago

Aye, aye, captain! … Triggered System Test worklow!

drogue-bot commented 2 years ago
Total Passed Failed Ignored Filtered Duration
✅ 147 147 0 0 0 30m 45s

Git: https://github.com/drogue-iot/drogue-cloud @ refs/heads/de-actix-web-ify

Commit: ff2c4bdc110204a2307634cdcc11d73df682a3a4
Author: Jim Crossley <jim@crossleys.org>
Date: Fri, 28 Jan 2022 02:12:38 +0500

    Refactor actix-web dependency to service-api only

    Initially, this is mostly re-exporting actix-web abstractions from
    service-api, but it can evolve to be smarter and more suited to how
    the drogue web apps use actix.

    The biggest challenge was the actix-web::main proc macro, since it
    generates code assuming actix-web is declared as a dep. Re-exporting
    it from service-api required implementing a new proc-macro crate:
    `macros`. Ugh.

    The new macros crate could possibly justify its existence by moving
    the `service-common/src/app.rs` stuff over to it, but that's for
    another PR.

    Removed actix-web dep from the following workspaces:
     service-common
     device-management-service
     console-backend
     user-auth-service
     coap-endpoint
     websocket-integration
     command-endpoint
     authentication-service
     integration-common (feature, too)
     admin-service (features, too)
     access-token-service
     database-common
     http-endpoint
     endpoint-common

    I initially attempted to put the re-exports in service-common, but
    because UserInformation is defined in service-api, its FromRequest
    impl needed to live there, too. So I reasoned it was ok for
    service-api to bring in the actix-web deps since most of the
    workspaces depend on both service-api and service-common anyway. This
    became a problem for console-frontend which targets wasm32, and some
    of the actix deps won't compile there.

    Therefore, we featurized the actix deps in service-api. Enabled by
    default, because all but the console-frontend can build the actix
    stuff.

Job: https://github.com/drogue-iot/drogue-cloud-testing/actions/runs/1758866116

Report: https://drogue-iot.github.io/drogue-cloud-testing/test-report/2022/01/27/test-run-1758866116.html