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

scripts(toolchain): several changes related to $TERMUX_PREFIX/include #23513

Merged

Conversation

robertkirkman
Copy link
Contributor

@robertkirkman robertkirkman commented Feb 26, 2025

  • This is the $TERMUX_PREFIX/include portion of [RFC] fix $TERMUX_PREFIX pollution #21835

  • Replace -I with -isystem in strategic locations: changes the include order of headers in $TERMUX_PREFIX/include relative to headers inside the source code of packages currently being built, so that if any headers inside the source code of the package being built have the exact same file names as headers inside $TERMUX_PREFIX/include, the headers from inside the source code of the package will be used, instead of the headers from inside $TERMUX_PREFIX/include, which would not be correct.

-DDEFAULT_SYSROOT=$(dirname $TERMUX_PREFIX/)

  • simulavr: the current simulavr build.sh runs mv $TERMUX_PREFIX/include ..., that is not really ideal because it effectively deletes the entire $TERMUX_PREFIX/include folder, preventing any other packages that might be built in the same container from using the headers they need from $TERMUX_PREFIX/include (and also potentially creating a malformed simulavr package .deb if any other packages were installed info $TERMUX_PREFIX beforehand). This rewrites the simulavr package in a way that avoids destroying $TERMUX_PREFIX/include.

@xtkoba I know you might not be here, but in case you see this, this would replace some of your code (in ghostscript) with other code, I would not want to remove your code like this without letting you know what is happening.

- This is the `$TERMUX_PREFIX/include` portion of termux#21835

- Replace `-I` with `-isystem` in strategic locations: changes the include order of headers in `$TERMUX_PREFIX/include` relative to headers inside the source code of packages currently being built, so that if any headers inside the source code of the package being built have the exact same file names as headers inside `$TERMUX_PREFIX/include`, the headers from inside the source code of the package will be used, instead of the headers from inside `$TERMUX_PREFIX/include`, which would not be correct.
  - Previous discussion was here https://github.com/termux/termux-packages/pull/22009/files/3dc455672cd67f05ed3cb814da07f7b9cfaf54af#diff-8c775c171f451b21463be103116ad5a4c2d0c54d3fb4ff4b59e987f98fc9c348
  - Prevents many _potential_ `$TERMUX_PREFIX/include`-related errors (that are often only reproducible when reusing a single docker container to compile multiple packages) in the following packages (and possibly other packages):
    - `nasm`
    - `bitlbee`
    - `emacs`
    - `gdb`
    - `libjxl`
    - `libmediainfo`
    - `maxcso`
    - `nodejs`
    - `pipewire`
    - `rust`
    - `sqlcipher`
    - `tome2`
    - `weggli`
    - `zip`
    - `proxmark3`
    - `handbrake`
  - This include order causes the `$TERMUX_ON_DEVICE_BUILD=false` build codepath to match the behavior of the `clang` binary from the `main/clang` package (i.e. the `$TERMUX_ON_DEVICE_BUILD=true` codepath), which is caused by the `-DDEFAULT_SYSROOT` setting that is built into it:

https://github.com/termux/termux-packages/blob/26a76f7b5de02c2b33c7f3d09c841712dfd4ccf0/packages/libllvm/build.sh#L40

- `simulavr`: the current `simulavr` `build.sh` runs `mv $TERMUX_PREFIX/include ...`, that is not really ideal because it effectively deletes the entire `$TERMUX_PREFIX/include` folder, preventing any other packages that might be built in the same container from using the headers they need from `$TERMUX_PREFIX/include` (and also potentially creating a malformed `simulavr` package `.deb` if any other packages were installed info `$TERMUX_PREFIX` beforehand). This rewrites the `simulavr` package in a way that avoids destroying `$TERMUX_PREFIX/include`.
@robertkirkman robertkirkman force-pushed the fix-crossbuild-prefix-pollution-include-1 branch from 6c438c9 to e2d4c5a Compare February 26, 2025 03:46
Copy link
Member

@twaik twaik left a comment

Choose a reason for hiding this comment

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

LGTM.

scripts/build/toolchain/* part of this PR seems to be harmless since it only lowers priority of termux rootfs include dir.

CC @truboxl @licy183 @sylirre

Copy link
Member

@licy183 licy183 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

All seems reasonable to me.

@twaik
Copy link
Member

twaik commented Feb 26, 2025

Ok, so it seems to be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants