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

feat: Add S3 backend support #2825

Open
wants to merge 68 commits into
base: main
Choose a base branch
from
Open

feat: Add S3 backend support #2825

wants to merge 68 commits into from

Conversation

delsner
Copy link
Contributor

@delsner delsner commented Jan 3, 2025

Motivation

Part of a series of PRs for implementing conda/rattler#960.

Changes

Add S3 backend support for pixi.toml and global pixi config.

Joint work with @moritzwilksch and @pavelzw.

@ruben-arts ruben-arts requested a review from baszalmstra January 6, 2025 10:29
@ruben-arts
Copy link
Contributor

Thanks guys, I'm asking @baszalmstra for a review of this work.

@delsner delsner changed the title Add S3 backend support feat: Add S3 backend support Jan 13, 2025
@pavelzw
Copy link
Contributor

pavelzw commented Jan 14, 2025

this first version should be ready for review as well, only a couple smaller things missing

@delsner delsner marked this pull request as ready for review January 14, 2025 12:23
@wolfv
Copy link
Member

wolfv commented Jan 14, 2025

fyi we temporarily removed the secrets to see if that fixes our release build.

Copy link
Contributor

@pavelzw pavelzw left a comment

Choose a reason for hiding this comment

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

from my side the PR is ready for another round of reviews @baszalmstra @ruben-arts @wolfv!

i'm currently still investigating why we get permission errors on windows when compiling but other than that it should be ready

/// Custom S3 configuration
#[derive(Debug, Clone, PartialEq, Serialize, Eq, Deserialize)]
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
pub struct S3Options {
Copy link
Contributor

Choose a reason for hiding this comment

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

currently we require to set all three values at once. At some point we might want to investigate defaults in the protocol but since it would be backwards compatible, i think this is fine for now

pub fn build_reqwest_clients(config: Option<&Config>) -> (Client, ClientWithMiddleware) {
pub fn build_reqwest_clients(
config: Option<&Config>,
s3_config_project: Option<HashMap<String, rattler_networking::s3_middleware::S3Config>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

we could pass an empty hashmap instead of None here as well but i thought it's a bit clearer in the API when we actually are using project config and when not like this

@@ -1,3 +1,4 @@
#![feature(once_cell_try)]
Copy link
Contributor

Choose a reason for hiding this comment

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

needed because of #2825 (comment), please let me know your thoughts on this, whether a nightly rust version is fine

Copy link
Contributor

@pavelzw pavelzw Jan 31, 2025

Choose a reason for hiding this comment

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

the failing windows tests are because for some reason cargo cannot create directories on the Dev drive E: created in #2566.

cargo build --locked --profile $env:CARGO_BUILD_PROFILE --features self_update
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    CARGO_INCREMENTAL: 0
    CARGO_NET_RETRY: 10
    RUSTUP_MAX_RETRIES: 10
    RUST_LOG: info
    RUST_BACKTRACE: 1
    RUSTFLAGS: -D warnings
    CARGO_TERM_COLOR: always
    CICD_INTERMEDIATES_DIR: _cicd-intermediates
    XDG_CACHE_HOME: C:\a\pixi\pixi/.cache
    PYTEST_ADDOPTS: --color=yes
    CARGO_BUILD_PROFILE: ci
    DEV_DRIVE: E:
    TMP: E:/pixi-tmp
    TEMP: E:/pixi-tmp
    RATTLER_CACHE_DIR: E:/rattler-cache
    RUSTUP_HOME: E:/.rustup
    CARGO_HOME: E:/.cargo
    PIXI_HOME: E:/.pixi
    PIXI_WORKSPACE: E:/pixi
    CACHE_ON_FAILURE: false
  
error: failed to create directory `E:\pixi\target`
Caused by:
  The request is not supported. (os error 50)

https://github.com/prefix-dev/pixi/actions/runs/13073287838/job/36479432799

my assumption would be that some internals changed with cargo 1.84.0 -> 1.86.0-nightly and that's why it doesn't work anymore. Should i revert the dev drive changes or do you have an other idea?

@@ -273,6 +285,7 @@ jobs:
- uses: Swatinem/rust-cache@v2
with:
workspaces: ${{ env.PIXI_WORKSPACE }}
- uses: mxschmitt/action-tmate@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- uses: mxschmitt/action-tmate@v3

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.

6 participants