-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Bug fix 12 #15
Bug fix 12 #15
Conversation
Bug fix 12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! A few comments/questions before I import it. Also, did you actually test the build after these changes on all three platforms -- or did you only test the commands?
src/packages/buskill/__init__.py
Outdated
|
||
if os.file.exists(APP_DIR, 'buskill.conf'): | ||
self.CONFIG = dict() | ||
config.read('buskill.conf') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be os.path( APP_DIR, 'buskill.conf' )
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello Matthew,
apologies I forgot I worked on this while waiting for a VM to come up.
The code is inactive I can amend this and re push code up.
actual feature progress is in work on a different branch on my fork
and yes the line should be if os.file.exists(os.path(APP_DIR, 'buskill.conf'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I was referring to the config.read()
line -- shouldn't that be config.read( os.path(APP_DIR,'buskill.conf') )
?
src/packages/buskill/__init__.py
Outdated
else: | ||
do_something = None | ||
# NOTE This a placeholder!!!! | ||
# Here there should be a setup wizard which will create the config file (TODO Make the function to create Config files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this suggest that currently the config file is not created if it doesn't yet exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a placeholder for now.
On local this has been actually implemented. will be pushed after testing
src/packages/buskill/__init__.py
Outdated
else: | ||
msg = "ERROR: Mac Kernel" + self.KERNEL_VERSION + "Unsupported" | ||
print( msg ); logger.error(msg) | ||
# subprocess.run( ['pmset', 'displaysleepnow'] ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, when does the pmset
command work? Wouldn't it be better to just try a bunch of commands as fall-back instead of giving-up before trying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the pmset command had the same issue which linked to the bug on my machines on the other version of macOS.
Also, it looks like there still some global var |
I will resolve this within the local code so will be resolved next PR |
thanks for the follow-up. Can you please fix the issues above before I merge this PR into the dev branch, instead of after in another PR? |
okay, closing PR Will create a new PR |
@stevenj2019 I--ummm--why close this PR? You could just push to the branch from which this PR was created, and that would update the existing PR without segmenting our comment history. Or am I missing something? |
honestly didn't know it worked like that, I was going to create a fresh PR without my awkward whoopsie in it 😆 |
I'm not a GitHub expert, but I think that's the process: we commit & comment & test iteratively in the same PR until it's good, and then the PR is accepted/merged.
I don't think mistaken commits matter if you removed it later in the PR. See the "Files changed" tab -- it's not even present, right? |
I pushed the changes to that branch, we should now see the changed here if that is correct. (I am a bit of a GitHub noob too) |
maybe try to unclose this PR? |
try another commit & push to |
well that worked, and I found more messy comments I forgot about so I will make another commit in a mo |
Another thing I want to confirm: are CI builds working for you? Can you go here to setup GitHub Actions on your repo so it actually builds the 3x executables that can be tested before I merge this into the main repo? |
I haven't managed to get them to work, manually nor via CI as long as it doesn't hit the main branch it should be okay. In future, if I could make branches on the main repo so I can take advantage of builds and such that would make life much easier, I do not have much time to contribute lately Is all |
What do you see when you go to this URL? Does it prompt you to enable GitHub Actions? What about here? |
https://github.com/stevenj2019/buskill-app/runs/1476954577?check_suite_focus=true#step:3:674 Linux build showed a warning but did not fail |
@stevenj2019 Looks like GitHub updated their MacOS shared runners base images, so python3.7 is no longer available (3.8 & 3.9 are). I updated the build script to be more robust in finding this binary going forward, and now it works:
Can you please pull from the upstream dev branch and merge that into your bug-fix-12 branch and try the build again in your repo? |
fixed build script
fixed build script
This commit makes the MacOS build script more robust so that it's able to survive when GitHub updates their MacOS shared runners python version (and deletes the old python binaires). For more info, see: * BusKill/buskill-app@ec4a023 * https://github.com/buskill/buskill-app/compare/abbb7a4415f0184e4594623d02b6386f856e2fb5..ec4a0239589cfa7b87432cb2e98e0db886abfaa5 * BusKill/buskill-app#15 (comment)
* #15 This commit adds a fall-back command and switches the subprocess function from 'call' to the newer 'run'
Pull Request,
Bugs discovered in issue have been fixed and tested on 17.x - 19.x mac os Kernels
Ready for push to dev