Skip to content
This repository was archived by the owner on Jan 23, 2026. It is now read-only.

Reogranize setup script#194

Merged
antoniofilipovic merged 17 commits intomainfrom
T560-FL-update-setup-to-fail
Jan 30, 2023
Merged

Reogranize setup script#194
antoniofilipovic merged 17 commits intomainfrom
T560-FL-update-setup-to-fail

Conversation

@antoniofilipovic
Copy link
Contributor

@antoniofilipovic antoniofilipovic commented Jan 19, 2023

Description

  • add logs
  • reorganize setup script to separate build for python, c++ and rust
  • remove "all" command
  • stop script if something fails

Pull request type

  • Feature

Related issues

Solves issue @tonilastre mentioned for Cloud

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

######################################

Copy link
Contributor

@Josipmrden Josipmrden left a comment

Choose a reason for hiding this comment

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

minor comment

setup Outdated
cmake_args.append(f"-D{MAGE_GPU_BUILD}")
if args.cpp_build_flags is not None:
cmake_args += [f"-D{flag}" for flag in args.cpp_build_flags]
if args["cpp_build_flags"] is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

extract args constants in enum or class with predefined values

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

The code looks good; suggesting several fixes to make the README more user-friendly and errors more helpful.

@antoniofilipovic antoniofilipovic self-assigned this Jan 25, 2023
@antoniofilipovic antoniofilipovic added the status: ready PR is ready for review label Jan 25, 2023
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Just notifying you about a few more issues I caught.

Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Just fix these bits before I approve! 🚀

Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Ship it 😁

@antepusic antepusic added status: ship it PR approved type: utility Something that everyone should use and removed status: ready PR is ready for review labels Jan 26, 2023
@antoniofilipovic antoniofilipovic merged commit 470a921 into main Jan 30, 2023
@antoniofilipovic antoniofilipovic deleted the T560-FL-update-setup-to-fail branch January 30, 2023 14:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

status: ship it PR approved type: utility Something that everyone should use

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants