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

try to fix more mirror things #146

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

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Jan 19, 2023

@Klaim it's dirty and doesn't work but I tried to clean up our mirror issues a bit.

Context ctx;
// TODO file URL handling is doing some weird things
auto target
= DownloadTarget::from_url(ctx, "file:///" + filename.string(), "out_zst.txt", ".");
Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhere we need to stop stripping a / ...

@@ -35,8 +35,10 @@ namespace powerloader
class mirror_map_type : private mirror_map_base
{
public:
using mirror_map_base::begin;
Copy link
Member

Choose a reason for hiding this comment

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

I initially voluntarilly didnt expose begin/end because that breaks the garantee that the values of the vector are unique (because then we can access and modify the vectors).
Suggestions: either

  1. use as_map() which makes a copy, not modifying this container;
  2. or expose the const versions of begin/end only.


ctx.mirror_map.create_unique_mirror<Mirror>(host, ctx, mirror_url);
ctx.mirror_map.create_unique_mirror<Mirror>(mirror_url, ctx, mirror_url);
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: the original code in the cli's main.cpp was using the variable host as key, which is why it was like that before. It doesnt change much of the meaning here but I'm not sure if it's ok.

{
dl_target->clear_base_url();
// TODO proper error handling here! :)
Copy link
Member

Choose a reason for hiding this comment

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

What error handling is missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not throwing runtime error but something more proper.

@wolfv wolfv mentioned this pull request Jan 21, 2023
@wolfv
Copy link
Member Author

wolfv commented Jan 22, 2023

I've merged most of the changes from this PR in smaller PRs over the weekend. There was a tricky segfault on Windows, but I think it was due to us not forcing Release builds. I did modify how we compute the MirrorID though, because that was what segfault'ed.
I think there are some minor function signature changes still in this PR that are not yet in main. Maybe you want to have a look and port them over to main @Klaim

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