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

[FDroidRepoBridge] Add New Bridge (closes #941) #2712

Merged
merged 2 commits into from
May 12, 2022

Conversation

yamanq
Copy link
Contributor

@yamanq yamanq commented May 9, 2022

This is a new bridge that parses any F-Droid repository. I'm not sure if the old bridge (which parses HTML from only f-droid.org, limited to 3 items) should be deleted in favor of this one. Closes #941.

Important: This requires enabling the zip extension, as all F-Droid repos are stored as jar files.

Given that a new extension is enabled, this PR should not be squashed so that the commit adding the extension is kept separate.

@github-actions
Copy link

github-actions bot commented May 9, 2022

Pull request artifacts

file last change
FDroidRepo-pr-context1 2022-05-12, 03:23:36
FDroidRepo-pr-context2 2022-05-12, 03:23:36

@yamanq yamanq force-pushed the fdroid branch 4 times, most recently from 1d6c580 to 0f31c26 Compare May 9, 2022 13:56
@dvikan
Copy link
Contributor

dvikan commented May 9, 2022

I upgraded phpunit in a separate commit.

@yamanq
Copy link
Contributor Author

yamanq commented May 9, 2022

Ok I will resolve the conflicts

@yamanq yamanq force-pushed the fdroid branch 2 times, most recently from b0fafc8 to cab0623 Compare May 9, 2022 20:45
@yamanq
Copy link
Contributor Author

yamanq commented May 9, 2022

I cleaned the commit history to just 2 commits: 1 enabling the extension and 1 adding the bridge, making it easier to commit without squashing.

I still would like feedback on adding a new requirement (zip) to RSS-Bridge @dvikan @Bockiii

@dvikan
Copy link
Contributor

dvikan commented May 9, 2022

We can do this in-memory:

$fp = $jar->getStream('index-v1.json');
while (!feof($fp)) {
  $contents .= fread($fp, 2);
}

Your code is doing caching ad-hoc and not using our cache libraries.

@yamanq
Copy link
Contributor Author

yamanq commented May 10, 2022

Sorry, I don't understand which part of the code your suggestion is meant to replace. I wanted to cache the index-v1.json file as it relatively large and should not change too often. How can I use the cache libraries here?

@dvikan
Copy link
Contributor

dvikan commented May 10, 2022

The jar file is a string of bytes. We can use our FileCache to cache it?

@dvikan
Copy link
Contributor

dvikan commented May 10, 2022

Ah our cache libraries has a max of 24h cache. Maybe thats why you cache it manually.

@yamanq
Copy link
Contributor Author

yamanq commented May 10, 2022

I understand why getRepo() should be rewritten. It assumes that the cache type is "file" when other backends are possible. To rewrite this, I will need to use a tmp directory to save the .jar file, extract the .json file, and read it. The resulting data can then be read into the cache using the recently added helper functions.

Is there a specific location that RSS-Bridge provides for temporary files?

@dvikan
Copy link
Contributor

dvikan commented May 11, 2022

You dont have to use a temporary file. Use $fp = $jar->getStream('index-v1.json');

@yamanq
Copy link
Contributor Author

yamanq commented May 11, 2022

Good idea, but I will still need the temporary file for the zip.

@dvikan
Copy link
Contributor

dvikan commented May 11, 2022

Ah yes maybe. Surprising that there is no option to open from string: https://www.php.net/manual/en/class.ziparchive.php

$tempfile=tempnam(sys_get_temp_dir(),''); seems good for a tempfile.

@yamanq
Copy link
Contributor Author

yamanq commented May 11, 2022

Now I am using only 1 temporary file for the zip and the bridge should be much faster since it is not constantly rereading the json into memory.

@yamanq
Copy link
Contributor Author

yamanq commented May 11, 2022

Rebased onto #2721 as this also modifies Dockerfile. Let's merge #2721 first.

@dvikan
Copy link
Contributor

dvikan commented May 11, 2022

Go ahead and do what you need to do. I'll backoff so there won't be any clashes.

@yamanq yamanq force-pushed the fdroid branch 2 times, most recently from 6574966 to 1cb47b2 Compare May 12, 2022 00:48
@yamanq
Copy link
Contributor Author

yamanq commented May 12, 2022

I Changed the exampleValue to a smaller repository so that it doesn't overflow the default memory limits. The error without this is:
<b>Fatal error</b>: Allowed memory size of 134217728 bytes exhausted (tried to allocate 23072768 bytes) in <b>/app/caches/FileCache.php</b> on line <b>29</b><br />

@dvikan Looking at my local files, the cache file for f-droid.org ends up being 25M, whereas the jar file is 6.4M. I'm inclined to merge this as is. What do you think the best way to proceed here is?

@dvikan
Copy link
Contributor

dvikan commented May 12, 2022

I think maybe it's not necessary to manually cache the json structure. The $jar = getContents($url . '/index-v1.jar'); already returns a cached result. Only problem is that getContents still does a slow network call.

@yamanq
Copy link
Contributor Author

yamanq commented May 12, 2022

This is an issue specifically with f-droid.org (See here). So I will keep remove the manual caching but keep the smaller repo as the example value, as it is less likely to be overloaded.

@yamanq yamanq merged commit 73b1a6a into RSS-Bridge:master May 12, 2022
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.

[FDroidBridge] suggesting enhancement for other repositories
2 participants