Closed ghost closed 9 months ago
It works perfectly fine when run using chmod +x make-pkgs.sh && ./make-pkgs.sh --ibus --fedora --develop --toolbox
But there are still alot of unfixed issues and edgecases
EDIT : MOST HAVE BEEN IRONED OUT
Notably :
fatal: destination path 'OpenBangla-Keyboard' already exists and is not an empty directory.
on subsequent runs.* bugs out with error `fatal: destination path 'OpenBangla-Keyboard' already exists and is not an empty directory.` on subsequent runs.
On that note, the fatal error occurs somewhere near line 11 of make-fedora.sh on subsequent runs I can't seem to find why it occurs, as the check should result in either entering the directory and doing a git pull or git cloning OBK if the directory doesnt exist, if anyone can help it would be appreciated.
EDIT : I think it has something to do with the git pull command expecting an empty directory or something similar, I am not experienced in dealing with git, so I can't fix the error in this case
EDIT2 : Fixed the bug, it now works fine
@ahmubashshir can you take a look?
@larina3315 Have you checked the scripts with shellcheck
?
@ahmubashshir can you take a look?
@larina3315 Have you checked the scripts with
shellcheck
?
Just checked, some suggestions showed up, I will update my scripts accordingly. I wanted to make a prototype/PoC ASAP, there are some things that can be improved, I will continue working on them
chmod +x make-pkgs.sh && ./make-pkgs.sh --ibus --fcitx --debian 11 --fedora 37 --toolbox --develop
chmod +x make-pkgs.sh && ./make-pkgs.sh --ibus --fcitx --debian 11 --fedora 37 --develop
chmod +x make-pkgs.sh && ./make-pkgs.sh --clean
chmod +x make-pkgs.sh && ./make-pkgs.sh --help
chmod +x make-pkgs.sh && ./make-pkgs.sh --hello
@larina3315 try amending the commits with meaningful messages.
https://www.conventionalcommits.org/en/v1.0.0/ https://github.com/joelparkerhenderson/git-commit-message/
@larina3315 try amending the commits with meaningful messages.
https://www.conventionalcommits.org/en/v1.0.0/ https://github.com/joelparkerhenderson/git-commit-message/
I am extremely sorry, I am still inexperienced in working with git. I have tried my best to squash related commits and give them better commit messages. I would like to edit the commit messages of some more commits, but so far neither Github nor Github Desktop seems to have the option to edit the commit message of a commit that was already pushed.
As for reviewing the PR, I would suggest focusing on/after Some cleanup and restructuring As it was a hefty restructure and reorganization of the entire thing, with the scripts being moved into their own folder
@larina3315 Okay the commits are good enough and I can squash them when merging this PR.
As you intend it to be a user-facing tool, there should be documentation about it. Can you add basic information about how should a user run it in the Readme.md
file?
Can you add basic information about how should a user run it in the
Readme.md
file?
yes, I will add a readme.md file for it asap.
I would like to ask for suggestion on whether stdout/stderr output of things like toolbox/dnf/apt etc should be
Let's keep current behavior as default and put others behind optiol… like --log-output=<file|always(default)|hide|on-error>
Let's keep current behavior as default and put others behind optiol… like
--log-output=<file|always(default)|hide|on-error>
The user would be bombarded with random things happening on screen, as long as things are going fine why bother the user ? I was thinking about making show on error the default
Although adding logging options can surely be done
I would like to ask for suggestion on whether stdout/stderr output of things like toolbox/dnf/apt etc should be
* suppressed * silently logged to a file * be shown to the user (current behaviour) * be shown ONLY on faliure
verbose output should be behind a -v
flag. But we can inform the user about the successes of the stages by outputting like Finished setting up dependencies
, Successfully built package
...etc.
I would like to ask for suggestion on whether stdout/stderr output of things like toolbox/dnf/apt etc should be
* suppressed * silently logged to a file * be shown to the user (current behaviour) * be shown ONLY on faliure
verbose output should be behind a
-v
flag. But we can inform the user about the successes of the stages by outputting likeFinished setting up dependencies
,Successfully built package
...etc.
I am currently looking into ways to introduce logging to bash scripts, although this change will take time as I will have to take a temporary hiatus due to academic reasons soon.
EDIT : Using set -x will log every command the shell tries to run
There can be some more QoL enchancements, code cleanups and documentation effort, but I feel like make-pkgs has reached it's intended state, adding more features will take it out of it's original scope of being an enduser focused tool
distrobox/docker support can also be easily added should anyone want to do such a thing, toolbox is a wrapper around podman so it already has podman support
Due to limitations of bash, I sadly cannot suppress log output for toolbox/dnf/cpack/git etc But the -v flag, when used as the first argument, will print and log every single line bash executes alongside stdout and stderr, this way if something does fail, we can just ask users to upload the *.log files
This is the best that could be done given the current circumstances, I had to put ALOT of ugly hacks to make it work. Ideally, I would've just written a rust program but the point of this is to be as portable as possible ("one-click") without platform/distro-specific issues, thus I had to use bash.
@larina3315 I appreciate your effort on this, thanks! :heart:
@ahmubashshir Do you have any review comments to add? Otherwise, I'd like to merge it.
@larina3315 I appreciate your effort on this, thanks! ❤️
@ahmubashshir Do you have any review comments to add? Otherwise, I'd like to merge it.
I have to write some misc. documentation regarding adding distro support and variables, some slight behaviour has changed which need to be mentioned in the readme
EDIT : I have finished doing appropriate changes
On Sat, Jan 13 2024 at 03:42:27 AM -08:00:00, Muhammad Mominul Huque @.***> wrote:
@ahmubashshir Do you have any review comments to add? Otherwise, I'd like to merge it.
LGTM, you can merge it.
- Mubashshir
Implements https://github.com/OpenBangla/OpenBangla-Keyboard/discussions/367