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

adding libcurl to support pause/resume download #1179

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

namchuai
Copy link
Contributor

@namchuai namchuai commented Sep 9, 2024

Describe Your Changes

  • Refactor, remove post processing download (extract, parse yaml, etc.) from download service.
  • Adding percentage download (default from libcurl)

Remaining things to do

  • Haven't tested with cortex update

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

@namchuai namchuai force-pushed the j/update-download-lib-to-curl branch 8 times, most recently from ff8c324 to df77be0 Compare September 12, 2024 01:12
@namchuai namchuai mentioned this pull request Sep 12, 2024
17 tasks
@namchuai namchuai force-pushed the j/update-download-lib-to-curl branch from a7b2aa9 to 9848c32 Compare September 12, 2024 17:46
@namchuai namchuai force-pushed the j/update-download-lib-to-curl branch 6 times, most recently from 6599960 to c6a0e28 Compare September 13, 2024 06:58
@namchuai namchuai changed the title [WIP] adding libcurl to support pause/resume download adding libcurl to support pause/resume download Sep 13, 2024
@namchuai namchuai marked this pull request as ready for review September 13, 2024 07:02
@namchuai namchuai force-pushed the j/update-download-lib-to-curl branch 3 times, most recently from a7b3069 to 7c9550b Compare September 13, 2024 09:12
@namchuai namchuai force-pushed the j/update-download-lib-to-curl branch from 7c9550b to c3c3946 Compare September 15, 2024 19:18
Copy link
Contributor

@dan-homebrew dan-homebrew left a comment

Choose a reason for hiding this comment

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

@namchuai I will let @vansangpfiev and @nguyenhoangthuan99 review this, but left a couple of comments on naming and architecture.

@@ -11,7 +10,7 @@ namespace commands {
#ifndef CORTEX_VARIANT
#define CORTEX_VARIANT file_manager_utils::kProdVariant
#endif
constexpr const auto kNightlyHost = "https://delta.jan.ai";
constexpr const auto kNightlyHost = "delta.jan.ai";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be pulled out into a .cortexrc? It seems very fragile to hard code a lot of these? cc @vansangpfiev @nguyenhoangthuan99

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.cortexrc should be for things that are configurable for user? I doubt it's a suitable place.
And, IMO, we are not hard-coded here. We are defining compile time constants.

engine/utils/cortexso_parser.h Show resolved Hide resolved
@namchuai namchuai merged commit 05d67be into dev Sep 16, 2024
4 checks passed
@namchuai namchuai deleted the j/update-download-lib-to-curl branch September 16, 2024 04:05
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