Skip to content
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

Fixes and improvements to scripts for working with cyclic dependencies #20513

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Maxython
Copy link
Member

@Maxython Maxython commented Jun 12, 2024

close #20338 - it is not possible to compile all packages using build-all.sh because buildorder.py cannot create build orders with circular dependencies.

There is also a solution to part of problem #20500:

Subpackages cause cycles in the build order.

There are also minor changes, all details are in the commit.

CC @tstein @TomJo2000

build-all.sh Show resolved Hide resolved
@Maxython Maxython force-pushed the fix-build-all branch 2 times, most recently from 9b5068d to 4317575 Compare June 13, 2024 14:35
@Maxython Maxython changed the title build-all.sh: fixes for working with cyclic dependencies Script improvements and fixes Jun 13, 2024
@Maxython Maxython force-pushed the fix-build-all branch 2 times, most recently from 59138be to c820cc0 Compare June 13, 2024 17:53
scripts/buildorder.py Outdated Show resolved Hide resolved
Copy link
Contributor

@tstein tstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me an hour to review this and I'm still not clear on what the new behavior is. We should not merge this until the code and comments are clear enough that reading it in one pass leaves you with a truthy understanding of the build order computation.

(I think it's a mistake to keep hacking on this without stepping back to think about how this code should be and how your changes get us closer. Not trying to block, because I think it's possible to get this to a state where it's better to merge it than not, but then we should have that conversation right after this.)

scripts/build/termux_step_setup_variables.sh Outdated Show resolved Hide resolved
scripts/build/termux_step_setup_variables.sh Outdated Show resolved Hide resolved
scripts/build/termux_step_setup_variables.sh Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
@agnostic-apollo

This comment was marked as off-topic.

@Maxython

This comment was marked as off-topic.

@agnostic-apollo

This comment was marked as off-topic.

@Maxython Maxython force-pushed the fix-build-all branch 5 times, most recently from 3dab149 to d46835f Compare June 17, 2024 15:23
@Maxython Maxython marked this pull request as ready for review June 17, 2024 15:23
@Maxython Maxython requested a review from Grimler91 June 17, 2024 15:23
build-all.sh Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
@Maxython Maxython changed the title Script improvements and fixes Scripts fixe and improvement Jun 18, 2024
@Maxython Maxython changed the title Scripts fixe and improvement Fixes and improvements to scripts for working with cyclic dependencies Jun 18, 2024
@Maxython Maxython requested a review from TomJo2000 June 18, 2024 11:46
@Maxython Maxython force-pushed the fix-build-all branch 2 times, most recently from 6b39ff1 to 5890600 Compare June 18, 2024 14:26
Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Max.

scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Show resolved Hide resolved
scripts/buildorder.py Show resolved Hide resolved
@Maxython
Copy link
Member Author

Do you have questions about changes to this PR? If not, then I think it can be safely merged to the master branch.

@TomJo2000
Copy link
Member

Let me give it another look.

Copy link
Member

@TomJo2000 TomJo2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple questions and readability nitpicks, but nothing major.

scripts/buildorder.py Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
scripts/buildorder.py Outdated Show resolved Hide resolved
@tstein
Copy link
Contributor

tstein commented Aug 12, 2024

Do you have questions about changes to this PR? If not, then I think it can be safely merged to the master branch.

Please stop suggesting that we can shortcut the review process here. As mentioned multiple times off-PR, you have anti-lgtm from me because I think the overall direction is wrong. If you can convince @TomJo2000 and @Grimler91 that this is an acceptable direction, I'll drop that objection, but I will make another pass for naming, comments, and python style. If we have to do it this way, the resulting code needs to be as readable as possible. (That will mean, at minimum, the clearest possible names for your new build vars, and the removal of most of those ternaries quoted in Tom's last pass.)

With this amount of attention on a change, you need explicit signoffs from everyone who got significantly involved. It can be merged to master when all three of us reach some agreement that it can be merged. Not before that.

@TomJo2000
Copy link
Member

I'm running build tests for NDK 27 right now and am using the opportunity to test ./build-all.sh.
Everything seems to be working as expected so far.

@Maxython
Copy link
Member Author

TODO: The package's base-library definition schema needs to be updated.

Copy link
Member

@Grimler91 Grimler91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an empty line between title and the rest of the text in the commit messages. Not having an empty line messes up for example git log --oneline:

b2291c89673e (HEAD -> fix-build-all, origin/fix-build-all) build-all.sh: improvements - added the ability to change the package format - added library base check by package name - the method of saving processes in the logo has been changed
bcd01a4ef87b termux_step_setup_variables.sh: adding new variables
c733e8207da0 Adding the ability to work with cyclic dependencies scripts/buildorder.py:  - the algorithm of the function `generate_full_buildorder` has been changed, now it can work with cyclic dependencies  - added new flag `-l` and function `get_list_cyclic_dependencies` which allows to find cyclic dependencies  - for subpackages a new variable `depend_on_parent` has been added which allows disabling dependencies on the parent package (controlled via the variable `TERMUX_SUBPKG_DEPEND_ON_PARENT`)  - updated logic of variable `only_installing`, now package dependency (with this variable) from variable `pkg_deps` will be used  - added the ability to control the `only_installing` variable for subpackages via the `TERMUX_SUBPKG_DEPEND_ON_PARENT` variable  - updated logic of the `recursive_dependencies` function, now only dependencies of the requested package will be returned during non-fast build scripts/build/termux_step_get_dependencies.sh: removed `-s` flag when compiling dependencies to fix cyclic dependencies build-all.sh: removed default `-s` flag when compiling packages and added check for compiled packages
73e7806901ab scripts/buildorder.py: adding new functions and fixing one algorithm - remove_char(var): removes unnecessary characters when parsing values - parse_build_file_variable(path, var): returns the value of a variable from the specified source - has_prefix_glibc(pkgname): checks if the package name has the prefix "glibc"
b675b06a8d2f scripts: improving work with cgct (#21112)
49fc92c95ad0 enhance(main/yazi): Remove unnecessary patch for tikv-jemalloc-sys crate
ceaccdef08ea bump(main/yazi): 0.3.1
034d49e3da10 revbump(main/ruby): Drop OpenMP build options
44f7fbe7d2fa bump(main/ruff): 0.6.0
c12ecb2955cc bump(main/libunbound): 1.21.0
b5fdd45ed9b3 bump(main/jfrog-cli): 2.63.2
...

exec 2> >(tee -a "$BUILDALL_DIR"/ALL.err >&2)
trap 'echo ERROR: See $BUILDALL_DIR/${PKG}.err' ERR
exec &> >(tee -a "$BUILDALL_DIR"/ALL.out)
trap 'echo ERROR: See $BUILDALL_DIR/${PKG}.out' ERR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not mentioned in commit message, and I am still not convinced changing how logs are created are a good idea. I think it should be in a separate PR so that it can be properly reviewed, and anyone that might be using it can chime in.

pkgname="${build_pkg##*/}"
(grep -q '^_' <<< "${pkgname}" || grep -q "^${pkgname}\$" "$BUILDSTATUS_FILE") && continue
echo "The \"${pkgname}\" package was also compiled"
echo "${pkgname}" >> "$BUILDSTATUS_FILE"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea. If build environment is not clean then the previously built packages will be added to buildstatus by this. That is most likely not what the builder wants.

I.e. if I run:

./build-all.sh -o ./special-build-dir/
# .... ctrl-c after X packages
./build-package.sh some-other-package
./build-all.sh -o ./special-build-dir/

Then some-other-package foo will be added to buildstatus file, but it will not be in special-build-dir/. I think it is reasonable for build-all.sh to build all packages, and not add extra packages it finds to the buildstatus file.

Copy link
Member

@TomJo2000 TomJo2000 Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think preserving the build status file would be very good to keep around as a commandline option.

I've been using that extensively as part of the #21130 build testing.

  • Run ./build-all.sh
  • Some package errors
  • Read the ${pkgname}.log, add it manually to the end of buildstatus.txt to skip it
  • Repeat

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a good idea. If build environment is not clean then the previously built packages will be added to buildstatus by this. That is most likely not what the builder wants.

I.e. if I run:

./build-all.sh -o ./special-build-dir/
# .... ctrl-c after X packages
./build-package.sh some-other-package
./build-all.sh -o ./special-build-dir/

Then some-other-package foo will be added to buildstatus file, but it will not be in special-build-dir/.

This example I absolutely agree with.
I've had to do a couple spot checks for individual packages out of order.
Running something like ./build-package.sh coreutils should not add to the buildstatus.txt file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check allows you to check if other packages have been compiled while other packages are being compiled. This mostly happens with package dependencies that have a circular dependency with the package. Thanks to this check, build-all.sh avoids recompiling the package.

I don't think this is a good idea. If build environment is not clean then the previously built packages will be added to buildstatus by this. That is most likely not what the builder wants.

Thanks for noting. I think that in the script build-all.sh I need to add the process of cleaning the environment (i.e. running the script clean.sh) before compiling all the packages. I'll think about how to solve this.

- remove_char(var): removes unnecessary characters when parsing values
- parse_build_file_variable(path, var): returns the value of a variable from the specified source
- has_prefix_glibc(pkgname): checks if the package name has the prefix "glibc"
scripts/buildorder.py:
 - the algorithm of the function `generate_full_buildorder` has been changed, now it can work with cyclic dependencies
 - added new flag `-l` and function `get_list_cyclic_dependencies` which allows to find cyclic dependencies
 - for subpackages a new variable `depend_on_parent` has been added which allows disabling dependencies on the parent package (controlled via the variable `TERMUX_SUBPKG_DEPEND_ON_PARENT`)
 - updated logic of variable `only_installing`, now package dependency (with this variable) from variable `pkg_deps` will be used
 - added the ability to control the `only_installing` variable for subpackages via the `TERMUX_SUBPKG_DEPEND_ON_PARENT` variable
 - updated logic of the `recursive_dependencies` function, now only dependencies of the requested package will be returned during non-fast build
scripts/build/termux_step_get_dependencies.sh:
 - removed `-s` flag when compiling dependencies to fix cyclic dependencies
 - fixed running of `termux_download_repo_file` function when cyclic dependencies are detected
build-package.sh: the algorithm that launches the signing key settings has been changed, now it will be launched if the value from the `TERMUX_REPO_PACKAGE` variable and from `TERMUX_APP_PACKAGE` match
build-all.sh: removed default `-s` flag when compiling packages and added check for compiled packages
- added the ability to change the package format
- added library base check by package name
- the method of saving processes in the logo has been changed
Virtual packages are packages that are compiled and installed into the system, but are not assembled into a source package for package managers. One advantage of virtual packages is that they can use sources (including values from configured variables) from regular packages. This will allow us to easily create special virtual packages that will have to resolve cyclic dependencies in regular packages.

PS: not ready for review yet, please wait

[no ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants