-
Notifications
You must be signed in to change notification settings - Fork 8
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
Create build script, update icons, update .desktop file, bump rev. #9
base: master
Are you sure you want to change the base?
Conversation
Thanks for the patchset, much appreciated, see feedback below.
1 . Just a minor complaint, can you update the commit descriptions to be a bit more descriptive?
At risk of coming off too critical, commit messages with "Update configure" or "Update Makefile.in" aren't too helpful to others looking through the history. If you need help with improving commit messages, there are many guides online to good git commit practices.
2. I noticed that you updated "xdg/xsensors.desktop" to point to "/usr/local/bin" instead of /usr/bin. I understand the rational for this change, as /usr/local/bin is used by default, but it would be better to update the makefile logic to template in "@bindir@" for this path. This way whatever path is provided to configure will correctly added to the desktop file.
E.g. when distros pick this up, they will be calling "./configure --bindir /usr/bin", but a user might want to use the default /usr/local/bin or even ~/.local/bin (where ~ is the home directory)
3. While I'm sure the build.sh script seems useful, I'd prefer to update the README to better explain how to build, clean, etc. To me it seems like an unnecessary wrapper.
If you want to update your patchset, please let me know. If not, I can cherry-pick/rework the relevant patches and close this.
|
@Mystro256 Sorry for the shitty commit message lol I understand. Yeah a bindir variable would be better. I dont wanna remove my build script. I have made a similar one for my forks of htop, nvtop, this repo, and geany. I frequently install linux distros on different machines and VMs, and it makes building and installing really quick for me. SOOO, I would prefer if you cherry pick. Later, I could update the readme with better instructions and make another pull request. |
@Mystro256 Updated my pull request
|
I use xsensors on the daily, and use this to get an updated version in i.e. Ubuntu (which comes with 0.70).
Added 16, 24, and 32 icon sizes, updated the .desktop file to point to the correct location and added keywords.
Created a build script that sets optimization flags, and prompts the user for --help, --clean, --gtk2, or --gtk3.
(Shows help, runs
make clean
, builds against GTK2, or builds against GTK3, respectively)