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 Installation Script for Windows (powershell) #312

Merged
merged 52 commits into from
Jun 29, 2023

Conversation

KrishPatel13
Copy link
Collaborator

@KrishPatel13 KrishPatel13 commented Jun 24, 2023

What kind of change does this PR introduce?

  • Remove VS C++ Redistributable Installation (not required anymore)
  • Organize the test script for readability
  • enhance User Oriented messages
  • refactoring code

Summary

Checklist

  • My code follows the style guidelines of OpenAdapt
  • I have performed a self-review of my code
  • If applicable, I have added tests to prove my fix is functional/effective
  • I have linted my code locally prior to submission
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. README.md, requirements.txt)
  • New and existing unit tests pass locally with my changes

How can your code be run and tested?
In Powershell: run the below command: (If Prompted, click 'Yes'):
Start-Process powershell -Verb RunAs -ArgumentList '-NoExit', '-ExecutionPolicy', 'Bypass', '-Command', "iwr -UseBasicParsing -Uri 'https://raw.githubusercontent.com/KrishPatel13/OpenAdapt/test/install/install/install_openadapt.ps1' | Invoke-Expression"

Other information

@KrishPatel13
Copy link
Collaborator Author

Also we should note that our main branch's poetry lock file and .toml files are not synced.

@abrichr
Copy link
Member

abrichr commented Jun 26, 2023

Thanks @KrishPatel13 !

Also we should note that our main branch's poetry lock file and .toml files are not synced.

Can you please clarify? Was this a local issue, or is there an error that you get on a fresh install?

@KrishPatel13
Copy link
Collaborator Author

It is on a fresh install. For everyone.

@abrichr
Copy link
Member

abrichr commented Jun 26, 2023

@KrishPatel13 can you please fix it and submit a PR? 🙏

@KrishPatel13
Copy link
Collaborator Author

@abrichr It is fixed in this commit: 79da291

of this PR.

Do you want a seprate PR for this particular commit?

@abrichr
Copy link
Member

abrichr commented Jun 26, 2023

Thank you @KrishPatel13 ! Separate PR is not needed. Is this ready for review?

@KrishPatel13
Copy link
Collaborator Author

KrishPatel13 commented Jun 26, 2023

@abrichr Yes, it is ready for review!

After this is merged, we can also merge the below PR:
OpenAdaptAI/OpenAdapt.web#27

@KrishPatel13 KrishPatel13 requested a review from dianzrong June 27, 2023 13:14
install/install_openadapt.ps1 Outdated Show resolved Hide resolved
install/install_openadapt.ps1 Show resolved Hide resolved
install/install_openadapt.ps1 Show resolved Hide resolved
install/install_openadapt.ps1 Show resolved Hide resolved
@KrishPatel13 KrishPatel13 requested a review from dianzrong June 27, 2023 16:30
@KrishPatel13
Copy link
Collaborator Author

@dianzrong Thank you for your review.

From #312 (comment) I would like to ask @abrichr, if this approach is vaible or not. Also somewhere I do not feel to ask for input from User as our ultimate goal is "One-Click (or press Enter) Install.

Any thought @dianzrong ? 🤔

Copy link
Collaborator

@dianzrong dianzrong 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, just some things I was confused about

install/install_openadapt.ps1 Outdated Show resolved Hide resolved
install/install_openadapt.ps1 Show resolved Hide resolved
install/install_openadapt.ps1 Outdated Show resolved Hide resolved
install/install_openadapt.ps1 Outdated Show resolved Hide resolved
@KrishPatel13
Copy link
Collaborator Author

@abrichr Ready for Merging!

Copy link
Collaborator

@dianzrong dianzrong left a comment

Choose a reason for hiding this comment

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

Looks great!!

@KrishPatel13
Copy link
Collaborator Author

Looks great!!

@dianzrong Thank you !!

@KrishPatel13 KrishPatel13 mentioned this pull request Jun 27, 2023
5 tasks
@KrishPatel13
Copy link
Collaborator Author

Trying to test again on VM, but gave same issue
image

@KrishPatel13
Copy link
Collaborator Author

@abrichr Ready for Review!

@abrichr abrichr merged commit ddb7d78 into OpenAdaptAI:main Jun 29, 2023
@abrichr abrichr deleted the test/install branch June 29, 2023 15:24
R-ohit-B-isht pushed a commit to R-ohit-B-isht/OpenAdapt that referenced this pull request Jun 21, 2024
* test only tesseract

* test only tesseract

* test tesseract only

* uncomment the last line

* try to redirect

* change the description for failed message

* add OCR

* test python

* format function calling

* Update Description

* prepare for git test

* test Git

* update the VSCppRedistCMD

* using Write-Host

* Use ';' for the return command

* full test

* add description for all arguments

* test tesseract on pc with out it

* fix order for the installtion

* uncomment the remove-gitinstaller

* Stop deletion of Openadapt if the pytest fails

* add the Cleanupfailure parameter to the apt comman

* make user oriented messages

* rewrite comments

* do not exit on pytest fail

* remove VS C++ Redistributable

* remove VS C++ installtion consts

* fix pytest fail exit

* redirect the output to null

* rename the param

* remove unneccessary line

* try a new powershell window for poetry shell

* add colours to messages

* try to fix NSIS error

* fix file name

* different approach to fix NSIS

* different approach to fix tesseract

* try to fic NSIS error

* try to fix NSIS error

* remove line

* remove uneccssary comment

* remove unsupported dcostrings

* remove unused arugs

* redirect output to null

* ran `poetry lock --no-update` to fix the warning

* Address comment:
OpenAdaptAI#312 (comment)

* Address comment:
OpenAdaptAI#312 (comment)

* Address comment:
OpenAdaptAI#312 (comment)

* imporve resuability and readability

* remove redirection to null
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.

[Bug]: Tesseract not installed Implement installation script for complete beginners
4 participants