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

Update sequencer #52

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update sequencer #52

wants to merge 2 commits into from

Conversation

Eagle941
Copy link
Contributor

@Eagle941 Eagle941 commented Oct 19, 2024

This PR updates blockifier replacing it with sequencer.

@Eagle941 Eagle941 changed the base branch from main to dict_test October 19, 2024 17:33
@Eagle941 Eagle941 force-pushed the update_sequencer branch 18 times, most recently from 24603df to 131ee48 Compare October 21, 2024 13:36
Base automatically changed from dict_test to main October 21, 2024 14:20
@Eagle941 Eagle941 force-pushed the update_sequencer branch 5 times, most recently from b556265 to d7b324d Compare October 21, 2024 21:36
@Eagle941 Eagle941 marked this pull request as ready for review October 22, 2024 15:47
@Eagle941 Eagle941 requested a review from iamrecursion October 22, 2024 15:47
Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Looks good, but a few questions/concerns that should be handled first.

Comment on lines +3 to +5
LLVM_SYS_181_PREFIX = "/usr/lib/llvm-18/"
MLIR_SYS_180_PREFIX = "/usr/lib/llvm-18/"
TABLEGEN_180_PREFIX = "/usr/lib/llvm-18/"
Copy link
Contributor

Choose a reason for hiding this comment

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

I have concerns about just requiring the user to have this built and installed. You could consider a nix-based approach like we use in llvm-to-cairo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script dependencies.sh performs the installation of llvm-18 for users that don't have it installed

README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,64 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Please comment this script.

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.

2 participants