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 readme. Add build instructions #69

Merged
merged 36 commits into from
May 21, 2024
Merged

Update readme. Add build instructions #69

merged 36 commits into from
May 21, 2024

Conversation

Menooker
Copy link

No description provided.

@Menooker
Copy link
Author

Depends on #66

@Menooker Menooker changed the base branch from main to yijie/dyn_link May 14, 2024 04:08
README.md Outdated
We have now installed LLVM at `llvm-project/llvm-install`.

Clone the graph-compiler repo:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is the readme file of this repo, we can skip the clone step.

Copy link
Contributor

@kurapov-peter kurapov-peter left a comment

Choose a reason for hiding this comment

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

Fine overall. I suggest adding the default build option with fetching llvm installation via gh. @leshikus, please take a look.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Base automatically changed from yijie/dyn_link to main May 15, 2024 03:57
@ciyongch
Copy link
Contributor

Please rebase.

@leshikus
Copy link
Contributor

leshikus commented May 17, 2024

I believe a shorter instruction to run scripts/compile.sh would be simpler. This also automatically installs llvm for a user

Why readme does not say what the graph compiler is for? I still don't know

@Menooker
Copy link
Author

Updates: recommend users to use the all-in-one script. Added a brief description for the project.

@leshikus
Copy link
Contributor

compile.sh still requires some dependencies to be installed first to work,

apt-get install -y --no-install-recommends --fix-missing gh; \

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Menooker Menooker merged commit 51ac048 into main May 21, 2024
3 checks passed
@Menooker
Copy link
Author

compile.sh still requires some dependencies to be installed first to work,

apt-get install -y --no-install-recommends --fix-missing gh; \

I am not sure the system requirements of using compile.sh. Let's update it later!

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.

6 participants