Firstyear / obs-service-cargo

OBS Source Service and utilities for Rust software packaging
Mozilla Public License 2.0
15 stars 9 forks source link

Migrate to filterer #48

Closed uncomfyhalomacro closed 9 months ago

uncomfyhalomacro commented 9 months ago

I may have to investigate

all-features: Enable all features of the current crate when vendoring.

after this PR. idk what that even means.

Closes #37

uncomfyhalomacro commented 9 months ago

Tested Rust project that also supports non-Linux platforms e.g. Windows, MacOS

uncomfyhalomacro commented 9 months ago

Alacritty be like

- Reduced vendored size using cargo-vendor-filterer
  * vendor.tar.xz was 28MB.. now it is 8.2MB
- Update _service file
  * replace mode "disabled" to "manual" since "disabled" is deprecated.

EDIT: But failed to build. Odd

uncomfyhalomacro commented 9 months ago

This build failure from Kanidm makes me think that filter should be an additional option

<services>
  <service name="cargo_vendor" mode="manual"/>
     <!-- If we want to vendor for *all* platforms even those from windows -->
     <param name="filter">*</param>
     <!-- If linux only -->
     <param name="filter">*linux*</param>
  </service>
  <service name="cargo_audit" mode="manual"/>
</services>

EDIT: I'm going to check tomorrow. Time to sleep.

Firstyear commented 9 months ago

I think wrt to the configuration, it should be <param name="filter">true|false</param> and we default to "true". I think we don't need to expose configuration knobs to users unless there is a need to let them actually configure it, and OBS doesn't build for anything that isn't linux anyway today. At least not that I'm aware of!

To start with, filterer might be more reliable ifwe just exclude windows, rather than try to filter to linux specifically?

uncomfyhalomacro commented 9 months ago

So filter should be

It should be good i guess. need to investigate further. im not sure how or why alacritty failed so I'm going to check if doing the "yolo" makes it happy.

uncomfyhalomacro commented 9 months ago

@Firstyear wow... alacritty builds when i set update false??????

Firstyear commented 9 months ago

It means alacritty deps on a specific version of something, but doesn't express it properly in it's Cargo.toml.

Anyway, I think you're right about the filter true/false thing,

uncomfyhalomacro commented 9 months ago

It means alacritty deps on a specific version of something, but doesn't express it properly in it's Cargo.toml.

Anyway, I think you're right about the filter true/false thing,

Also found out. With the regular cargo vendor after cargo update:

With the cargo-vendor-filterer with filter glob *-unknown-linux-gnu after cargo update:

uncomfyhalomacro commented 9 months ago

@Firstyear Can you please confirm it's working if you are free 🫂? For now, I will just ignore that inconsistent behavior between cargo vendor vs cargo vendor-filterer.

uncomfyhalomacro commented 9 months ago

Not going to merge this yet. Here are inconsistent behaviors I found with Kani

First scenario

We have options:

🛑 Failed to build!

Second scenario

We have options:

🛑 Failed to build!

Third scenario

We have options:

🛑 Failed to build

What were the errors?

Same errors?

[  143s] error[E0433]: failed to resolve: could not find `interface_types` in `tss_esapi`
[  143s]     --> libs/crypto/src/lib.rs:1179:20
[  143s]      |
[  143s] 1179 |     use tss_esapi::interface_types::algorithm::HashingAlgorithm;
[  143s]      |                    ^^^^^^^^^^^^^^^ could not find `interface_types` in `tss_esapi`
[  143s] 
[  143s] error[E0432]: unresolved imports `tss_esapi::handles`, `tss_esapi::Context`, `tss_esapi::Error`
[  143s]   --> libs/crypto/src/lib.rs:32:21
[  143s]    |
[  143s] 32 | pub use tss_esapi::{handles::ObjectHandle as TpmHandle, Context as TpmContext, Error as TpmError};
[  143s]    |                     ^^^^^^^                             ^^^^^^^^^^^^^^^^^^^^^  ^^^^^^^^^^^^^^^^^ no `Error` in the root
[  143s]    |                     |                                   |
[  143s]    |                     |                                   no `Context` in the root
[  143s]    |                     could not find `handles` in `tss_esapi`
[  143s]    |
[  143s]    = help: consider importing one of these items instead:
[  143s]            std::task::Context
[  143s]            core::task::Context
[  143s]    = help: consider importing one of these items instead:
[  143s]            argon2::Error
[  143s]            crate::fmt::Error
[  143s]            openssl::error::Error
[  143s]            openssl::ssl::Error
[  143s]            rand::Error
[  143s]            serde::__private::doc::Error
[  143s]            serde::__private::fmt::Error
[  143s]            serde::de::Error
[  143s]            serde::de::value::Error
[  143s]            serde::ser::Error
[  143s]            std::error::Error
[  143s]            std::fmt::Error
[  143s]            std::io::Error
[  143s]            tracing::log::Level::Error
[  143s]            tracing::log::LevelFilter::Error
[  143s]            core::error::Error
[  143s]            core::fmt::Error

Now what?

Cargo vendor filterer seems it's not ready to be merged. Thoughts @Firstyear ?

uncomfyhalomacro commented 9 months ago

image I wonder why it fails to do so? is it because of *-windows-*? that's weird

Firstyear commented 9 months ago

Cargo vendor filterer seems it's not ready to be merged. Thoughts @Firstyear ?

vendor filter is "nice to have" not required. We have other ways to reduce archive size (like default compression levels etc).

So I think we opt to skip this for now since it's causing problems, and right now our goal needs to be a "smooth replacement" of the python to rust versions. Throwing in more variables only risks that change.

uncomfyhalomacro commented 9 months ago

Yeah I will keep the branch. for now it's dead.