-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
addpkg(x11/gnucash): 5.13 #25962
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
addpkg(x11/gnucash): 5.13 #25962
Conversation
|
This is my first attempt to add or modify a Termux package, so sorry in advance if I screwed something up! I wasn't exactly sure how to handle the license field, because has different files under different licenses: https://github.com/Gnucash/gnucash/blob/stable/LICENSE So I just marked the license as |
TomJo2000
left a comment
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 looking good for a first time contribution.
I've left you a couple pointers as review comments.
try adding |
I approve of your patch and I believe it is correct. Eventually, a similar change will also be made in upstream Gnucash, which you could open a PR for as well, but since the necessary change is very small in this case, I expect that it will probably be straightforward for upstream to make the change on their own without too much trouble. |
Others can clarify the Termux package license policies in more detail if needed, but I believe that what you have done (specifying "custom" and adding a comment that lists the licenses) is one acceptable way, but if you wanted to, there is also another way to specify license in Termux packages that can actually distribute the license files with the package itself in the folder I believe that this other package where I have done that serves as a good example of how to do it, in case you would like to try: The paths which are listed in termux-packages/packages/aosp-libs/build.sh Lines 3 to 14 in be9b63a
if some licenses are originally stored in files with the same names (like in my example), Termux will automatically rename each additional license that conflicts with one of the other license filenames as the same name but with " |
This is what I proposed in #25962 (comment), As a tangent, here's the code that handles installation of multiple license files.
They're installed as termux-packages/scripts/build/termux_step_install_license.sh Lines 61 to 70 in be9b63a
Though it appears that aosp-libs is a non-standard case here as it's license files are in fact named NOTICE.$COUNT.I assume this is because the codepath for explicitly specified license locations doesn't rename them to copyright.$COUNTtermux-packages/scripts/build/termux_step_install_license.sh Lines 10 to 32 in be9b63a
|
|
@TomJo2000 @robertkirkman Thank you for all the information and feedback! I made all your suggested changes. Now I'm just trying to figure out what's going on with the |
I am assuming there is a missing build option along the lines of
Builds in the package build container and on-device can sometimes have different behavior. ./scripts/run-docker.sh ./build-package.sh -I -f gnucash |
|
I'm going to try to make a solution to the next problem in CI, I believe that if given the choice, Tomjo2000 will prefer solutions involving the addition of the build dependency however, I still like to write things the way that does not involve use of those things, sometimes. I believe that I would like to show acozzette both ways. |
Whatever gets the job done. |
Just would like to give an update that after checking some things, it absolutely will be much easier and less code to use |
|
An additional update is: I have now successfully built Therefore, if you are very eager to see a solution ASAP and don't want to wait longer, please let me know and I can immediately upload and show my first solution so that you don't have to wait. However, at the moment, unfortunately I would be embarrassed to show my first version that is messy. Now that I know both ways of building
|
The two possible methods for fixing the error "
|
| # run test suite inside proot | |
| termux-proot-run env LD_PRELOAD= LD_LIBRARY_PATH= \ | |
| CC="$CC" CFLAGS="$CFLAGS" CPPFLAGS="$CPPFLAGS" LINKFLAGS="$LDFLAGS" \ | |
| TMPDIR="$TERMUX_PREFIX/tmp" \ | |
| sh run-tests.sh |
Full code of this method for gnucash: robertkirkman@edd2202
Build log of this example in termux-packages GitHub Actions CI: https://github.com/robertkirkman/termux-packages/actions/runs/17515294855/job/49752007407
Total package source code length, not including imported code that is shared between multiple termux packages: 344 lines
|
Both methods are available to use and you can feel free to copy and paste them from the commits I uploaded and linked. You can feel free to decide which method is more appealing to you after reading the information and code I have provided and testing them as you see fit. |
|
@robertkirkman Wow, that's awesome! Thanks so much for doing all this. Your explanations help a lot to understand the build issues involved. I think I'm leaning toward preferring your Method 2, since the necessary changes look a bit less invasive and since this method offers the ability of potentially getting the tests working. If I can find time this weekend then I'll try to start from your commit and see if I can get the tests to run. Do you think it's important to enable Python support? I'm just curious because I would be slightly inclined to leave it turned off. I don't personally plan on using it and thought it might simplify things a little bit to leave it disabled. |
not really, i just enabled it because it's not difficult to do so and usually we do enable python support in other packages. if you want to leave it off you can.
sure, I found the errors that happen when enabling the testsuite difficult, but I can also try helping with that and i understand the importance of it, since enabling the tests in another package SBCL did help identify and fix multiple bugs in that software that only appeared when it was built for Android and especially Android-x86 that would not really have been possible to fix otherwise. I think that these two topics will probably be relevant for getting the tests to pass in CI: debugging and troubleshooting Scheme code and the |
|
@robertkirkman Sorry for taking forever to get back to this. I haven't forgotten about it but have just been busy with other things. I was thinking that since you did almost all the real work of getting this working, you should feel free to submit your own PR if you want. If not then I will try to get back to it soon, but probably won't be able to work on it for another week at least. |
That's ok, and sure, I have also been planning to eventually try some more to get the tests to work. Part of my plan involves writing code that would simplify running build-time tests like what gnucash has, in a way that would be easier to generalize to many packages, rather than dependent on large, package-specific patches. That plan could be described as adding a "third" build method, distinct from the other two build methods that I have showed examples of here so far. This PR is a blocker for me to make progress on that third method, so I'm waiting for reviews on it. If approved, eventually it would allow me to implement the third method of building, which should be much more easily compatible with build-time testsuites and much easier to maintain. |
|
I finally got back to this and updated my PR. I copied Robert's termux-proot changes and also upgraded to Gnucash 5.13 since that has been released in the meantime. I was able to build the package successfully in Docker and install and run it on my device, so hopefully this is now ready to merge. |
|
Hm, it looks like the builds for 32-bit targets failed. I will take a look and see if I can fix that or not. |
|
When I test building gnucash for 32-bit Android in non-cross-compilation mode (meaning, building on a real Android device with a real 32-bit CPU), unfortunately the same problem happens, with identical warnings and errors. That narrows down the problem to something going wrong on specifically 32-bit Android with either gnucash itself, or with the 32-bit Termux build of the It's acceptable as a last resort to disable the 32-bit targets in Termux packages and implement the package only for 64-bit targets, but if at all possible it's desirable to make packages work on 32-bit targets, since otherwise people who have 32-bit devices won't be able to get them, so I will continue trying. |
|
I have figured out that this error when running the 32-bit build of This also explains why I didn't see this error when I originally wrote the build script for gnucash that you are trying to use - at that time, Termux It was only later that I upgraded the Termux however, while doing that fixed a bug elsewhere, it seems like it might have introduced a bug that prevents building I could start bisecting the issue, but I've also noticed that Debian's https://sources.debian.org/src/guile-3.0/3.0.10%2Breally3.0.10-6/debian/patches I'll probably try applying the Debian patches to Termux's |
|
After a lot of testing, I have collected the following information:
|
|
Wow, well thanks for doing all this work to debug the problem! I'll hold off for now and wait for your fix to land. |
|
Ok, @acozzette I have merged my PR now which fixed the 32-bit Termux |
|
@robertkirkman Great, thank you! I just rebased this PR onto master. |
|
I can see that there is now another, different error, which definitely was not happening 3 days ago when I thought I tested the exact PR I merged combined with this PR locally. I must not have tested it as completely as I thought I did, though, because when I test again, I can reproduce the new error when only the first bisected commit is reverted, but not when using a development snapshot of the entire Guile project at the commit right before the bisected commit. I really was sure that I tested the final version of my recent Termux There is a way to reproduce that first error on GNU/Linux, which I documented in full in my recent issue associated with my Termux However, it appears that there might be more commits necessary to revert than just that one in order to build The additional commits necessary to revert are definitely somewhere in between the commit that caused the first error and the final release of Guile 3.0.10, because this second error is definitely not reproducible with the upstream commit of Guile immediately before the commit that caused the first error, I have just tested that again to reconfirm that today. I will now find the additional commit or commits in guile 3.0.10 that are responsible for the second error and any further errors. |
|
I've written and, this time, I believe fully tested all the necessary additional changes to the I did bisect the second error to several other upstream Guile commits, however, this time, I tried again one of the workarounds that doesn't involve reverting commits which I tried on the first error that didn't work at the time, and now that the first error is fixed, that same workaround is now able to work and this time it's actually helping since it appears to fix the second error, meaning that I probably don't need to revert any additional commits, but instead only add all that additional code to the When I test repeatedly building both |
- Fixes multiple malformations in the 32-bit builds of `guile` 3.0.10, including but not limited to this MRE of a `WARNING`: ``` ~ $ cat hello.scm !# (display "Hello, world!") (newline) ~ $ ./hello.scm ;;; WARNING: compilation of /data/data/com.termux/files/home/./hello.scm failed: ;;; In procedure load-thunk-from-memory: ELF file does not have native word size Hello, world! ~ $ ``` - Necessary after these three upstream Guile commits and other commits which depend on them: - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=137b0e85b956a7b7e7c9bc0029d09fe6868e5b17 - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=c758c99b5e37408e48dc1b22c73d6ec35d9de866 - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=88e0933450ed9f1cc96858641e0756ffa44c53c6 - Tangentially related follow-up to #26977, but **that PR is still necessary also, this PR does not replace it**. - Necessary to complete #25962
- Fixes multiple malformations in the 32-bit builds of `guile` 3.0.10, including but not limited to this MRE of a `WARNING`: ``` ~ $ cat hello.scm !# (display "Hello, world!") (newline) ~ $ ./hello.scm ;;; WARNING: compilation of /data/data/com.termux/files/home/./hello.scm failed: ;;; In procedure load-thunk-from-memory: ELF file does not have native word size Hello, world! ~ $ ``` - Necessary after these three upstream Guile commits and other commits which depend on them: - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=137b0e85b956a7b7e7c9bc0029d09fe6868e5b17 - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=c758c99b5e37408e48dc1b22c73d6ec35d9de866 - https://cgit.git.savannah.gnu.org/cgit/guile.git/commit/?id=88e0933450ed9f1cc96858641e0756ffa44c53c6 - Tangentially related follow-up to termux/termux-packages#26977, but **that PR is still necessary also, this PR does not replace it**. - Necessary to complete termux/termux-packages#25962
Credit goes to @robertkirkman for doing most of the hard work of actually getting this to build! I largely copied Robert's changes with permission. Fixes termux#20259.
robertkirkman
left a comment
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.
I think it's ready, I will merge it in 24 hours if no additional problems are found.
Credit goes to @robertkirkman for doing most of the hard work of actually getting this to build! I largely copied Robert's changes with permission.
Fixes #20259.