Repository navigation
keep scan thread for version before high sierra - #728
Conversation
d4ce5f2 to
8235a2b
Compare
|
@ylluminarious not really, it seems that the issue is for versions 10.13.2 to 10.13.3 is it ? |
|
@pmalhaire For me the issue started with macOS High Sierra 10.13.0 and persisted in all versions until 10.13.3. With the public beta of 10.13.4 that was released recently (and htop 2.0.2) I couldn't reproduce the issue anymore. |
bba9153 to
c100f4e
Compare
|
What is the value of |
|
@ilovezfs On 10.13.3 it's |
|
@pmalhaire so this PR also needs to exclude 17.4.0 |
|
@pmalhaire and >= 17.5.0 is safe again. |
c100f4e to
d9af224
Compare
|
Thx I mixed up. Fixer @ilovezfs @ylluminarious |
|
Infortunately I don't have version to test the fix |
|
@pmalhaire Thanks! Hopefully someone who is on the beta can try your patch and see if it works. Also @hishamhm should probably review this for merging. |
| #include <stdbool.h> | ||
|
|
||
|
|
||
| void GetKernelVersion(int ** major, int ** minor, int ** internal) { |
There was a problem hiding this comment.
Is the ** needed here? I may be wrong, but it seems like you only need int *major etc. here if you change the bottom lines to be *major = cMajor; etc. and the variable declarations to be int maj, min, inter;.
There was a problem hiding this comment.
If I want to use pointers it's needed or the value won't be set.
Since the size of a pointer is the same or bigger than an int, I could use simple pointer.
d9af224 to
5e9c38b
Compare
|
I have a VM running High Sierra with one snapshot on 10.13.3 and one snapshot on 10.13.4 public beta. I'd be happy to run tests if it's of any help but I'd need some guidance on how and what exactly to test. I could build htop from this branch ( |
|
@anothernode 10.13.3 state (S) has ? Line 302 in 8a147dd |
|
@pmalhaire All right, thanks! So I compiled htop from As you can see in the attached screenshots, on 10.13.3, the High Sierra 10.13.3: High Sierra 10.13.4 public beta: |
|
Seems fine @anothernode |
|
Hey @pmalhaire! Thank you for this patch! I'm wondering though if it's worth keeping a lot of version detection code around to catch one bug in one specific point release... is 10.13.4 about to be released? I think once that is out, basically all High Sierra users will upgrade to it, right? So at that point, reenabling the feature unconditionally would make sense. What do you think? (I don't have a big understanding of typical macOS user upgrade patterns) |
|
@hishamhm You are right most people will upgrade to a newer version. I didn't find updated upgrade rate but I'd say from past experiences that 30% of users will not upgrade 6 month after the upgrade is released. Blocking it at package manager level (brew) is an other option what do you think @ilovezfs ? Unfortunately macOs bug rate is quite high at the time. It may change, but you may need this kind of code again. I'll compact my code anyway, it's too long for what it does. |
e645e37 to
08dbac0
Compare
|
@hishamhm @pmalhaire I know a fair number of people who do not upgrade right when a new release comes out. Even if they're already on the most recent major version. I am not one of those people most of the time, but I don't think that having some extra lines will hurt anything, considering the fact that people often have a good reason not to upgrade to a new minor version right away. Downtime, compatibility, bugs in the new version are all valid concerns and I think it's a bad practice to impose such minor versions on users, when it's not even much of a pain for the developer himself. |
|
Thanks @ylluminarious for your explaination. @hishamhm Given what @ylluminarious added I'd suggest merging this code. |
|
@hishamhm Thank you very much for merging this! I'm sure we can all appreciate the solution given by @pmalhaire. |
|
@ilovezfs The homebrew formula for |
Keep scanning threads for versions before High Sierra 13.0.0 and after 13.3.0.
|
@ilovezfs @pmalhaire @hishamhm @anothernode Yikes, sorry folks, looks like this patch did not fix the issue. According to my own testing, this bug is still alive and well on High Sierra versions lower than 10.13.4. |
|
I've reverted this patch from 2.1.0 in Homebrew: Homebrew/homebrew-core#25887 until we figure out what went wrong here. @pmalhaire I think the comparisons are backwards here. Won't |
|
@ilovezfs @pmalhaire Sounds like a fine situation for good ol' |
|
@ylluminarious are you suggesting we not guess? |
|
@ilovezfs Heheh, well... yes :) I've no problem with testing the reverse comparison patch that you made, but it sure wouldn't hurt to test it both ways and write some sort of logging statement indicating whether the main boolean expression is doing what we expect it to. Just as a sanity check, if nothing else. |
|
Er, yes, the comparisons are is back-to-front, aren't they? Copy and pasting into an isolated program: #include <stdbool.h> [0/76]
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/sysctl.h>
struct kern {
short int version[3];
};
void GetKernelVersion(struct kern *k) {
static short int version_[3] = {0};
if (!version_[0]) {
// just in case it fails someday
version_[0] = version_[1] = version_[2] = -1;
char str[256] = {0};
size_t size = sizeof(str);
int ret = sysctlbyname("kern.osrelease", str, &size, NULL, 0);
if (ret == 0) sscanf(str, "%hd.%hd.%hd", &version_[0], &version_[1], &version_[2]);
}
memcpy(k->version, version_, sizeof(version_));
}
int CompareKernelVersion(short int major, short int minor, short int component) {
struct kern k;
GetKernelVersion(&k);
if ( k.version[0] != major) return major - k.version[0];
if ( k.version[1] != minor) return minor - k.version[1];
if ( k.version[2] != component) return component - k.version[2];
return 0;
}
bool foo() {
bool isScanThreadSupported = ! ( CompareKernelVersion(17, 0, 0) >= 0 && CompareKernelVersion(17, 5, 0) < 0);
return isScanThreadSupported;
}
int main(void) {
puts(foo() ? "true" : "false");
return EXIT_SUCCESS;
}This tells me "true" and I'm on macOS 10.13.3 so it should be telling me "false." Should this be the following? bool isScanThreadSupported = ! ( CompareKernelVersion(17, 0, 0) < 0 && CompareKernelVersion(17, 5, 0) >= 0); |
|
No, I think it needs to be otherwise 10.13.4 will be disqualified too. Or we can reverse the subtraction order in CompareKernelVersion. |
|
@ilovezfs @Smattr I know this still sounds elementary and obvious... but this is a place where I would use After some unsuccessful testing in a VM, I'd like to have more confidence before being a guinea pig again. I am sure that others would like the same assurance. I would do it myself, but I have not had time to go and compile |
|
@ylluminarious I did, which is how I identified the off-by-one error in @Smattr's suggestion. But please feel free to verify my findings yourself :) |
|
@ilovezfs Oh cool, that's great. I didn't realize you had already done that. Thank you, I have some more confidence to try it out now. I suppose I can try it out at some point tomorrow or the next day unless someone beats me to it :) |
|
@ylluminarious please note that my suggestion above is different from what I said in the other issue, which is what accounts for the failure monouser7dig still saw. |
|
@ilovezfs Right, I noticed that it was different. Thank you for further clarifying that. |
|
I'll have a look |
|
I definetly switched the test while compacting my code, I'll fix and send a test executable tonight. |
|
Personally I'm happy with always running So maybe we could also consider a kind of patch where It kind of seems to be a bit of a waste of effort to put much more time and energy into this, now that we know that 10.13.4 will fix the actual root cause of the whole mess... |
|
@anothernode It's cool that you have root access on your computer. Many users don't. |
|
test.c.txt Sorry for the error. |
|
Apple has now released 10.13.4 |
|
Ok, so I attempted to test @ilovezfs's fix yesterday, but I had some issues connecting to https://hisham.hm, so I could not get the modified formula to install. Moreover, I noticed that @pmalhaire started a new PR at #772, so which patch should be tested now? |
|
@ylluminarious @ilovezfs the patch I made just fixes the error I made. It's the same as what @ilovezfs suggested. |
|
@pmalhaire Ok, your solution is slightly different in that it changes the arithmetic in |
|
@pmalhaire @ilovezfs @hishamhm Looks like #772 does the job and fixes this problem for good. |
|
@ilovezfs @hishamhm To reiterate a comment I left on #682 and to avoid any confusion: I left |
Keep scanning threads for versions before High Sierra 13.0.0 and after 13.3.0.
@hishamhm As said this should do the trick to keep thread info for those who don't have High Sierra.
Unfortunately I don't know the exact version failling.