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

Simplify end user set up instructions #138

Closed

Conversation

sfc-gh-cromano
Copy link

This change aims to simplify the setup process for users by removing the usage of
make, which is not available on all systems. The Makefile still exists and can still
be used, but it is not explicitly mentioned in the README for setup instructions. There
are also several differing solutions
on how to install Make on Windows, which could introduce unnecessary steps and
confusion for users.

In addition to this, the "dist/" files have been removed from the repository and the
folder has been added to the .gitignore file. It's fairly easy to generate these
distribution files and it could potentially add another step to users that have not
installed packages from a wheel file. Installing the package from the root of the
repository via pip install . or pip install -e . may be easier to digest for users
and will reduce the repository size.

Removed make file instructions and added direct instructions
@sfc-gh-jsummer
Copy link
Collaborator

I see the makefile command to run the app in the README. I think adding alternatives (sans Make) is fine but let's certainly keep the makefile implementation and instructions.

Copy link
Collaborator

@sfc-gh-cnivera sfc-gh-cnivera left a comment

Choose a reason for hiding this comment

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

Thanks! This set of changes makes sense, if it's easier for users to install the package by pip installing from root then we don't need the whl. I do think we should keep the Make targets in the README if possible unless you feel strongly that it'd be confusing, perhaps we can reword it to say that these commands are available if you already have Make installed.

README.md Show resolved Hide resolved
Copy link
Collaborator

@sfc-gh-twhite sfc-gh-twhite left a comment

Choose a reason for hiding this comment

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

I think we should determine how best to move forward with keeping or removing the "make" instructions from the README.

I don't find much benefit in keeping both options here, as the "make" commands are only replacing what are already trivial commands. In my experience, some users are just simply going to execute each command without thoroughly reading, and leaving both options could result in redundant executions. Worse, if they try "make" and it fails, they may not dig deeper and troubleshoot why. I'm all for keeping the "make" commands available via the Makefile.

To users that are less familiar with how "make" works, even seeing it could cause confusion. I think this would be a different story if we could provide these instructions in a "carousel" format, but reading top-down for alternatives isn't as clear.

Makefile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
sfc-gh-cromano and others added 2 commits August 29, 2024 09:32
Co-authored-by: Tyler White <[email protected]>
remove whl steps
@sfc-gh-cnivera
Copy link
Collaborator

Just combing through the repo - figuring we can close this for now since we're on the brink of doing a setup revamp w/ SiS support. Feel free to reopen if I've misunderstood.

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.

4 participants