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

Fix installation with git via pip #37

Conversation

zbeucler2018
Copy link
Collaborator

Fixes the bug in this issue

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

I don't like this change, doesn't it mean that if it fails, the project will have no __version__?

Could you try using the os.path.abspath(__file__) to see if this fixes the path name

@zbeucler2018
Copy link
Collaborator Author

@pseudo-rnd-thoughts I'm a little confused, could you emphasize further? The reason I had to wrap this in a try/except is because when the project is installed via pip install *.whl or pip install git+... , the retro/VERSION file isnt included in the install, which was the cause of the original bug

@pseudo-rnd-thoughts
Copy link
Member

To my understanding, the file is rather retro/../VERSION, i.e., just VERSION in the root folder
We might need to update

"VERSION.txt",
to VERSION rather than VERSION.txt but this seems unrelated

@@ -8,8 +8,14 @@
ROOT_DIR = os.path.abspath(os.path.dirname(__file__))
core_path(os.path.join(os.path.dirname(__file__), "cores"))

with open(os.path.join(os.path.dirname(__file__), "../VERSION")) as f:
__version__ = f.read()
try:
Copy link
Member

Choose a reason for hiding this comment

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

Given that we have fixed this bug, could we remove the try except and the extra if statement at the end?

Otherwise, looks good

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pseudo-rnd-thoughts unfortunately the bug is still present. The reason is because there is no VERSION file included in the install at all. I've included the VERSION file in MANIFEST.in and made the setup.py edit you suggested and still no luck. What is odd is that the Version field of pip show stable-retro is populated with the correct value. Any ideas? Please see the screenshot for reference

Screenshot 2023-07-03 at 7 07 13 PM

@pseudo-rnd-thoughts
Copy link
Member

Trying the colab, I found that none of the package data was included, therefore, I think there is an issue with that.
I found the workaround was to put the VERSION file in retro only.
Previously, the project had two version files however to me this seems like a stupid idea that can cause issues and is easier to have a single file for this.

@zbeucler2018
Copy link
Collaborator Author

I agree, two version files is a bad idea. I can implement that workaround later today

@zbeucler2018
Copy link
Collaborator Author

zbeucler2018 commented Jul 15, 2023

Issue has been fixed. @pseudo-rnd-thoughts please take another look when you have a free second, thank you!
Screenshot 2023-07-14 at 10 59 07 PM

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit f7f7c9d into Farama-Foundation:master Jul 15, 2023
6 checks passed
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.

None yet

2 participants