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

Add ABI dump script #94135

Merged
merged 1 commit into from Jun 23, 2022
Merged

Add ABI dump script #94135

merged 1 commit into from Jun 23, 2022

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 22, 2022

No description provided.

@pablogsal pablogsal force-pushed the abidump-script branch 2 times, most recently from 8692028 to a48758e Compare Jun 22, 2022
@pablogsal pablogsal changed the title Add ABI dump script and Makefile target Add ABI dump script Jun 22, 2022
Copy link
Member

@encukou encukou left a comment

Looks good, two little improvements below.

I checked with podman run -v$(pwd):/src:Z -w /src --rm -it ubuntu:20.04 bash /src/.github/workflows/regen-abidump.sh. Two changes to carry over to the Docker command line:

  • -w /src for the working directory
  • --rm to remove the container:
    • If you aren't familiar with containers you probably don't want used ones lying around.
    • If you do want to keep it around, you probably know what --rm is and can remove it.

.github/workflows/regen-abidump.sh Outdated Show resolved Hide resolved
.github/workflows/regen-abidump.sh Show resolved Hide resolved
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Jun 23, 2022

Thanks a lot for the review @encukou

@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Jun 23, 2022

I will make the PR to fix the devguide soon

@pablogsal pablogsal merged commit 8e6ecca into python:main Jun 23, 2022
13 checks passed
@pablogsal pablogsal deleted the abidump-script branch Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants