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

fix(CI): Keep Xtensa clang environment variables off the general system #491

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

chrysn
Copy link
Collaborator

@chrysn chrysn commented Oct 26, 2024

Description

The Xtensa toolchain sets a general LIBCLANG_PATH, which is part of what causes the clang troubles for libOSCORE (to be fixed in #443).

This approach unsets it from the GitHub action environment, and will later set it specifically for the ESP builds on demand.

[edit: added] Now we can't unset it from the GitHub actions because that just doesn't work, but we prepare a sourceable .profile style file that is then sourced by all relevant run scripts.

Open Questions

  • Will it build?
    [edit, added:] … and it didn't even need that environment variable anywhere else 🤦

  • Were alternatives considered?
    Yes: The alternative is to fork the Xtensa script and make it optional to export the variable.
    Or fork GitHub runners and fix Allow to unset env variables actions/runner#1126 so that we can just unset what the action sets.
    But those would all be way more effort than maintaining this workaround.

PR dependencies

Depended on #492

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

@chrysn chrysn mentioned this pull request Oct 26, 2024
4 tasks
chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this pull request Oct 26, 2024
@chrysn chrysn force-pushed the xtensa-dont-mess-with-env branch 2 times, most recently from 57a0f14 to 766330c Compare October 26, 2024 23:15
@chrysn
Copy link
Collaborator Author

chrysn commented Oct 26, 2024

Turned out it did build, so the LIBCLANG_PATH (which is now stored away in _XTENSA_LIBCLANG_PATH in case anything we do needs to pull it back in) was exported completely needlessly and just broke stuff.

I've rebased this PR onto #492 – originally they were distinct approaches, but with how that one turned out to just update the system, and how this one turned out to just work, both contribute to having a consistent story towards resolving the CoAP build issues (which eventually #443 should fix).

@chrysn chrysn marked this pull request as ready for review October 26, 2024 23:36
@chrysn chrysn marked this pull request as draft October 27, 2024 07:39
@chrysn chrysn force-pushed the xtensa-dont-mess-with-env branch 4 times, most recently from 8ef483b to 8672b76 Compare October 27, 2024 08:13
@chrysn
Copy link
Collaborator Author

chrysn commented Oct 27, 2024

Set back to draft when I found that as tracked in actions/runner#1126, GitHub action environment variables can simply not be unset in general.

@chrysn chrysn force-pushed the xtensa-dont-mess-with-env branch from 8672b76 to caf286d Compare October 27, 2024 08:18
@chrysn chrysn force-pushed the xtensa-dont-mess-with-env branch from caf286d to 1751b0c Compare October 27, 2024 08:22
@chrysn
Copy link
Collaborator Author

chrysn commented Oct 27, 2024

This is now ready for review again -- it finally does something, and conveniently, the (back then unfounded) observation that the environment variable is not really needed held.

@chrysn chrysn marked this pull request as ready for review October 27, 2024 09:00
Copy link
Collaborator

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Damn ugly but pragmatic. Maybe make it generic, call it 'unset-variables.sh' or use an even more generic name for "sourced always first" script that we can just do "ecgi TWEAK >> name" into?

@kaspar030
Copy link
Collaborator

Lgtm

@chrysn
Copy link
Collaborator Author

chrysn commented Oct 27, 2024

If we don't go with a squash, #443 would work as-is, and I don't think it hurts to keep this one, but if you prefer squashing, I can do that too.

@chrysn chrysn added this pull request to the merge queue Oct 27, 2024
Merged via the queue into ariel-os:main with commit 0aff01a Oct 27, 2024
26 checks passed
@chrysn chrysn deleted the xtensa-dont-mess-with-env branch October 27, 2024 11:29
@ROMemories ROMemories added the CI label Oct 28, 2024
chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this pull request Feb 17, 2025
With [38] tentatively in (through a branched action), [491] can be
reverted; the latter had doing something like [38] in its list of
considered alternatives, but failed to foresee the need for sourcing all
that data back in for compiling C code on Xtensa.

[38]: esp-rs/xtensa-toolchain#38
[491]: ariel-os#491
chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this pull request Feb 17, 2025
With [38] tentatively in (through a branched action), [491] can be
reverted; the latter had doing something like [38] in its list of
considered alternatives, but failed to foresee the need for sourcing all
that data back in for compiling C code on Xtensa.

[38]: esp-rs/xtensa-toolchain#38
[491]: ariel-os#491
chrysn added a commit to chrysn-pull-requests/RIOT-rs that referenced this pull request Feb 17, 2025
With [38] tentatively in (through a branched action), [491] can be
reverted; the latter had doing something like [38] in its list of
considered alternatives, but failed to foresee the need for sourcing all
that data back in for compiling C code on Xtensa.

[38]: esp-rs/xtensa-toolchain#38
[491]: ariel-os#491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to unset env variables
3 participants