- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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 BuildPythonAPI cleaning and success message #2348
Fix BuildPythonAPI cleaning and success message #2348
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update our CHANGELOG.md based on your changes. |
Hi @jeffreykxiao ! And I am still missing |
Co-Authored-By: Gökhan Barış Aker <gokhanbarisaker@gmail.com>
That problem is unrelated to this PR (which only attempts to fix a message + make clean functionality), so I think it would be best to create a new issue. Additionally when doing so, adding some more context would be great! Also don't forget that the Discord server is available to help in real time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bernatx and @gokhanbarisaker)
@jeffreykxiao Thanks for your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bernatx and @gokhanbarisaker)
Util/BuildTools/BuildPythonAPI.bat, line 156 at r1 (raw file):
Previously, jeffreykxiao (Jeff Xiao) wrote…
Hm, it seems I did. IDK if the whole repo attempts to maintain posix compliance but nonetheless unnecessary changes are bad so I will gladly accept this
(accepted change on github, but forgot to resolve on reviewable)
Hi @bernatx, just wanted to ping you real quick about this one since it's a fairly easy one just waiting for your testing / approval :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, thanks for your effort.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @bernatx and @gokhanbarisaker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gokhanbarisaker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @gokhanbarisaker from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved
Hi all, I have what it looks like the same problem as @veliona .
However I have the new version with the code reviewed by @jeffreykxiao and I don't get why I have this problem. |
Hi, I also encounter the same issue, have you solved it now? |
Description
BuildPythonAPI.bat was broken because it was expecting
%PYTHON_LIB_PATH%
to have a trailing slash, which it was missing.Fixes #2339
Behavior prior to this change:
make clean
does not clean the intermediate files for building PythonAPImake PythonAPI
displays the wrong path for the build resultBehavior with this change:
make clean
properly cleans up the intermediate files located inPythonAPI/carla/build
,PythonAPI/carla/dist
, andPythonAPI/carla/source/carla.egg-info
make PythonAPI
reports the correct location of the build resultWhere has this been tested?
Possible Drawbacks
Now that
make clean
actually attempts to clean up the intermediate build artifacts, in case they are held by some other process,make clean
may fail to clean up some of the directories. This is bog standard for Windows so as long as people are paying attention to the output ofmake clean
all should be well.Existing issue: code does not check for existing installs, so we can still end up with multiple egg files in
dist
if we do notmake clean
This change is