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

print status to console; handle prefix change #17

Closed
wants to merge 3 commits into from

Conversation

segfault-bilibili
Copy link

@segfault-bilibili segfault-bilibili commented Oct 23, 2022

I found that sometimes generate-db.js seems to unexpectedly skip some files which haven't yet finished downloading. It seems to be a silent error as well.

I then tried to fix this, although I still don't know why that could happen at all.

By the way, as you can see, I also did a refactor to separate fetchFile as a function.

@segfault-bilibili
Copy link
Author

I'm sorry. I misunderstood the situation.

The real root cause was the fact that I changed TERMUX_APP_PACKAGE in scripts/properties.sh so that the prefix check here will never get passed:

let linesContainingPathToBinaries = ungzipped
.split("\n")
.filter((line) => {
return line.startsWith(binPrefix);
});

.. and then forEach will never do anything because the array is empty:

Object.keys(binMap)
.sort()
.forEach((packageName) => {
fs.appendFileSync(headerFile, `"${packageName}",\n`);
binMap[packageName].sort().forEach((packageName) => {
fs.appendFileSync(headerFile, `" ${packageName}",\n`);
});
});

generate-db.js Outdated
}
);
const encoding = "utf-8";
fs.writeFileSync(headerFile, content, encoding);
Copy link
Author

Choose a reason for hiding this comment

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

Currently my incorrect "fix" did nothing but creating empty files so that the compiler won't refuse to continue. However this obviously is not a proper fix at all.

@segfault-bilibili segfault-bilibili changed the title fix unexpectedly skipped (missing) file; print status to console print status to console; handle prefix change Oct 23, 2022
@segfault-bilibili segfault-bilibili marked this pull request as ready for review October 23, 2022 14:22
@segfault-bilibili
Copy link
Author

I've updated my patch. However I'm not sure whether it's correct or not.

Copy link
Member

@thunder-coding thunder-coding left a comment

Choose a reason for hiding this comment

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

Kindly also rebase against master since huge parts of this script have been rewritten.

const path = require("node:path");
const zlib = require("node:zlib");

const repositoryURL = "https://packages-cf.termux.dev/apt";
// TODO(@thunder-coding): Do not hardcode list of known architectures.
const archs = ["aarch64", "arm", "i686", "x86_64"];
const scriptdir = process.env["TERMUX_SCRIPTDIR"];
const defaultPrefixRegExp = /^data\/data\/com\.termux\/files\/usr/g;
Copy link
Member

Choose a reason for hiding this comment

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

Bad idea to use regex, if simple string comparision works well. Also hardcodes the prefix

Copy link
Author

Choose a reason for hiding this comment

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

Also hardcodes prefix

Oh I think it's used to replace the hardcoded default prefix? Maybe something has changed as time goes by.

}
);
if (content.replace(/\n/g, "") === "") throw new Error(`content is empty`);
Copy link
Member

Choose a reason for hiding this comment

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

Same suggestion, don't use regex

@@ -1,13 +1,14 @@
#!/usr/bin/env node
const fs = require("node:fs");
const https = require("node:https");
Copy link
Member

Choose a reason for hiding this comment

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

Why are you moving from https to http2? http2 does not require encryption https://http2.github.io/faq/#does-http2-require-encryption which can be a problem

Copy link
Author

Choose a reason for hiding this comment

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

Oh I didn't go deeper with that. I think the URL is still https so it is still encrypted. Did I get anything wrong?

@segfault-bilibili
Copy link
Author

Honestly I'm considering to close this pr now.

The comments of this pr discussion thread now look confusing even to myself. It was composed quite some time ago. It was probably composed on a dirty tree instead of clean one as well.

The initial problem I wanted to fix turned out to be just a mistake introduced by myself. That's why I edited the initial comments on this discussion thread, putting strikethrough on the text.

However at that time I still thought that this pr maybe still make some sense, therefore I decided to leave it here.

@segfault-bilibili
Copy link
Author

segfault-bilibili commented Sep 17, 2023

The hardcoded prefix issue seems to be rooted at very fundamental level of termux project as I l'm getting less and less unfamiliar with this project.

I now think it's reasonable to support dynamic prefix as this would fix multiuser support.

However this is probably not very easy to be done.

The only way I'm currently aware of, is a hackish way to use a default prefix with placeholding characters, so that it can be replaced with /user/12345 at installation time, thus, this text replacement is done without changing the length of string. termux/termux-app#2734

I wrote "handle prefix change" in the title, however it just replaces the prefix from one hardcoded string to another hardcoded string. It's still not a fix to this problem. It not even doing the hackish installation-time text replacement mentioned above. (however it still makes some sense in case that someone just needs to change the hardcoded prefix, instead of moving towards dynamic prefix)

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