Skip to content

JIT README: How to install LLVM on Fedora Linux #118983

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 16, 2024
Merged

JIT README: How to install LLVM on Fedora Linux #118983

merged 2 commits into from
May 16, 2024

Conversation

hroncok
Copy link
Contributor

@hroncok hroncok commented May 13, 2024

@hroncok hroncok requested a review from brandtbucher as a code owner May 13, 2024 07:51
@hroncok
Copy link
Contributor Author

hroncok commented May 13, 2024

As a side note, I've also inquired about having LLVM 18 available in Fedora Linux 39.

@Eclips4
Copy link
Member

Eclips4 commented May 13, 2024 •
edited
Loading

Miro, is this really should target 3.13 branch? I guess you have chose this one accidentally.
For me, it looks like it should target the current main branch (3.14), and it would be nice to backport it to 3.13, since JIT is available in 3.13.

@hroncok hroncok changed the base branch from 3.13 to main May 13, 2024 10:32
@Eclips4 Eclips4 added the needs backport to 3.13 bugs and security fixes label May 13, 2024
@Eclips4
Copy link
Member

Eclips4 commented May 13, 2024 •
edited
Loading

Miro, is this really should target 3.13 branch? I guess you have chose this one accidentally. For me, it looks like it should target the current main branch (3.14), and it would be nice to backport it to 3.13, since JIT is available in 3.13.

Nah, I was doing this on my phone from GitHub web edit and I managed to click the edit button on the 3.13 branch, sorry about that.

Now I belive I fixed this, but at the same time, GitHub decided to spam everybody. I am so sorry 😢

No worries! Bad things happens sometimes with Github UI/UX :). I've dismissed requests for review from everyone except Brandt.

@Eclips4
Copy link
Member

Eclips4 commented May 13, 2024 •
edited
Loading

However, I'm wondering why changing README.md triggers our CI/CD actions (I mean actions that build the interpreter and run the test suite). This obviously should not happen for this kind of changes.

This could be easily fixed by changing the .github/workflows/jit.yml:

diff --git a/.github/workflows/jit.yml b/.github/workflows/jit.yml
index 7152cde8f4..1ac2829f0a 100644
--- a/.github/workflows/jit.yml
+++ b/.github/workflows/jit.yml
@@ -1,6 +1,8 @@
 name: JIT
 on:
   pull_request:
+    paths-ignore:
+      - 'Tools/jit/README.md'
     paths:
       - '**jit**'
       - 'Python/bytecodes.c'

Perhaps it's not worth the effort because README is rarely changed.

savannahostrowski
savannahostrowski approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for adding this!

ericsnowcurrently
ericsnowcurrently approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@ericsnowcurrently
Copy link
Member

However, I'm wondering why changing README.md triggers our CI/CD actions (I mean actions that build the interpreter and run the test suite). This obviously should not happen for this kind of changes.

This could be easily fixed by changing the .github/workflows/jit.yml:

That makes sense to me. @brandtbucher, what do you think?

erlend-aasland
erlend-aasland reviewed May 14, 2024
View reviewed changes
@hugovk
Copy link
Member

hugovk commented May 14, 2024

This could be easily fixed by changing the .github/workflows/jit.yml:

Could also ignore Tools/jit/mypy.ini

@hroncok @erlend-aasland
Mark the code-block as shell
0af61e0 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
brandtbucher
brandtbucher approved these changes May 16, 2024
View reviewed changes
Tools/jit/README.md
Install LLVM 18 on Fedora Linux 40 or newer:

```sh
sudo dnf install 'clang(major) = 18' 'llvm(major) = 18'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this! Just double-checking, is the separate Clang install necessary (it's not bundled with the LLVM install)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Installing just LLVM does not install clang. Neither the other way around.

@brandtbucher
Copy link
Member

Also, if somebody wants to open a PR to ignore JIT CI on the following files, go ahead:

  • Tools/jit/README.md
  • Tools/jit/mypy.ini
  • Python/perf_jit_trampoline.c

@savannahostrowski
Copy link
Member

Happy to do that! @brandtbucher

@brandtbucher brandtbucher merged commit ab73bcd into python:main May 16, 2024
57 checks passed
@miss-islington-app
Copy link

Thanks @hroncok for the PR, and @brandtbucher for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 16, 2024
@hroncok @erlend-aasland @miss-islington
Explain how to install LLVM on Fedora (pythonGH-118983)
98bedc5 
(cherry picked from commit ab73bcd)

Co-authored-by: Miro Hrončok <miro@hroncok.cz>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-app
Copy link

bedevere-app bot commented May 16, 2024

GH-119100 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 16, 2024
@hroncok hroncok deleted the patch-2 branch May 16, 2024 18:09
@savannahostrowski savannahostrowski mentioned this pull request May 18, 2024
Closed
@Eclips4 Eclips4 mentioned this pull request Jun 12, 2024
Merged
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@hroncok @erlend-aasland @estyxx
Explain how to install LLVM on Fedora (pythonGH-118983)
474b094 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@savannahostrowski savannahostrowski savannahostrowski approved these changes

@brandtbucher brandtbucher brandtbucher approved these changes

Assignees
No one assigned
Labels
skip issue skip news topic-JIT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hroncok @Eclips4 @ericsnowcurrently @hugovk @brandtbucher @savannahostrowski @erlend-aasland