Skip to content

Recursive download flag/general improvements in handling downloads#403

Draft
ammernico wants to merge 4 commits into
science-computing:masterfrom
ammernico:download
Draft

Recursive download flag/general improvements in handling downloads#403
ammernico wants to merge 4 commits into
science-computing:masterfrom
ammernico:download

Conversation

@ammernico

Copy link
Copy Markdown
Contributor

No description provided.

@ammernico
ammernico requested a review from primeos-work August 13, 2024 13:21

@primeos-work primeos-work left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I reviewed this commit by commit so the comments might be a bit irritating since you've switched to another approach in a later commit.

Nitpick regarding the "Move XYZ into a own function" commit messages: I'd use "dedicated function" instead.

TODO: I still have to review the last commit.

Comment thread src/commands/source/download.rs Outdated
Comment thread src/commands/source/download.rs Outdated
Comment thread src/commands/source/download.rs Outdated
Comment thread src/commands/source/download.rs Outdated
Comment thread src/commands/source/download.rs
Comment thread src/commands/source/download.rs Outdated
Comment thread Cargo.toml Outdated
Comment thread Cargo.lock Outdated
Comment thread src/commands/source/download.rs Outdated
@ammernico
ammernico marked this pull request as draft August 27, 2024 14:09
@ammernico
ammernico marked this pull request as ready for review September 4, 2024 17:03
@ammernico
ammernico marked this pull request as draft September 12, 2024 10:02
@ammernico
ammernico force-pushed the download branch 3 times, most recently from 0438b72 to aaa7fd1 Compare September 12, 2024 16:37
@christophprokop christophprokop added the prerelease PRs which are merged to staging branch but not in main/master label Oct 1, 2024
Nico Steinle added 4 commits October 14, 2024 15:29
I found the show_message a bit misleading since indicatif has the same
method name for setting a real message. The method on the wrapper was
just showing the progress.

https://docs.rs/indicatif/0.17.8/indicatif/struct.ProgressBar.html#method.set_message

Signed-off-by: Nico Steinle <nico.steinle@eviden.com>
Refactor source download logic into a separate function for better
code organization.

Signed-off-by: Nico Steinle <nico.steinle@eviden.com>
This refactor improves the maintainability and clarity of the
download function.

Signed-off-by: Nico Steinle <nico.steinle@eviden.com>
- Track and display download results in an ASCII table

- Make `source already exists` not an error, just a result type

- Introduced a `--recursive` flag to the `download` subcommand to
  enable downloading of both the main packages and all their
  dependencies.

- Added `--image` and `--env` options to specify the Docker image
  and environment variables, ensuring the correct dependency tree
  is resolved based on the build environment. Took this approach
  from the tree-of command.

- Used a `HashSet` to avoid duplicate processing of packages and
  their dependencies.

- Only check sources where the download was successful, skipped or
  forced. Use the `verify_impl` to verify the sources. Calling
  `super::verify` wasn't ideal since it could break on different CLI
  arguments.

This update improves clarity and tracking of the download outcomes
and allows for more comprehensive package management by
ensuring that all necessary dependencies are downloaded alongside
the requested packages.

Signed-off-by: Nico Steinle <nico.steinle@eviden.com>
@ammernico ammernico removed the prerelease PRs which are merged to staging branch but not in main/master label Oct 16, 2024
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