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 aim-downloader #996

Merged
merged 8 commits into from
Dec 10, 2023
Merged

Conversation

darknight
Copy link
Contributor

Follow up of #948

As @wsxiaoys suggested in the comment, we can maintain a cuszomized verson of aim to keep using it.

Luckily, aim is MIT license and codebase is not big, so I did following changes:

  • created aim-downloader, copy aim source code to this crate
  • removed main.rs & driver.rs, removed skim-navi dependence
    • main.rs provides command-line interface, which is not needed as a lib
    • driver.rs is only used by main.rs, so removed
      • http_serve_folder.rs is only used in driver.rs, which enables aim serve a folder over http. It's useless when aim as a lib, so removed. Also removed its dependence: warpy = "0.3.35"
    • skim-navi is only used by driver.rs and incompatible with windows, so removed: skim-navi = "0.1.10"
  • based on aim's feature matrix, s3 & ssh download does not support resume, which is why we use this lib. So they're useless, removed them and related test data
  • re-implement untildify, another incompatible lib with windows. untildify is a one-function crate which convert a tilde path to its full path(e.g. ~/.ssh to /home/user/.ssh). It's also MIT license, so I copied unit tests from its codebase, instead of copying it's implementation, I just re-wrote the function which defined in untildify.rs.

With these changes, together with the changes from #948 applied in tabby, I'm able to build tabby successfully regardless of which build mode.

And I'm able to run download without issue:
Release mode:
Screenshot 2023-12-09 200759
Debug mode:
Screenshot 2023-12-09 202907

Pending work:

  • Shall we keep narrowing down aim to support http download only? (so we don't check-in unused code)

@darknight darknight marked this pull request as draft December 9, 2023 13:53
@darknight darknight changed the title Add aim downloader feat: add aim-downloader Dec 9, 2023
@wsxiaoys
Copy link
Member

wsxiaoys commented Dec 9, 2023

Since we only utilize HttpsHandler, shall we remove all other handles?

Besides - could you cleanup all windows build related change in this PR?

assert_eq!(untildify("Desktop"), "Desktop");
assert_eq!(untildify(""), "");
assert_eq!(untildify("/"), "/");
// assert_eq!(untildify("~/Desktop/~/Code"), "/User/Untildify/Desktop/");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original implementation used regex to capture the first tilde, then truncate the rest string.
Current implementation does not check if tilde exists in the middle of string, so this case will fail.

@wsxiaoys
Copy link
Member

wsxiaoys commented Dec 9, 2023

since we don't run any integration tests for aim - we can remove the tests dir?

@wsxiaoys wsxiaoys marked this pull request as ready for review December 10, 2023 01:18
@darknight
Copy link
Contributor Author

since we don't run any integration tests for aim - we can remove the tests dir?

One more thing, some of the tests require network access to test http handler, for example: get_https_works, shall we disable it as well?

}
}

#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test requires network access

std::fs::remove_file(out_file).unwrap();
}

#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test requires network access and external test file

std::fs::remove_file(out_file).unwrap();
}

#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

assert_eq!(result, expected);
}

#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above, add ignore to skip.

std::fs::remove_file(".netrc.test_unit").unwrap();
}

#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test failed then running on windows in the last step remove_file call - cannot find the file specified.

run individually is fine. I suspect race condition with above test considering cargo test run in parallel.

So move test logic from this test case to previous one, and disable this.

@wsxiaoys wsxiaoys merged commit 1fa0106 into TabbyML:main Dec 10, 2023
3 checks passed
@darknight darknight deleted the add-aim-downloader branch December 10, 2023 02:53
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.

2 participants