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

clean ffi/build with setup.py clean #569

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ARF1
Copy link
Contributor

@ARF1 ARF1 commented Apr 1, 2020

Fixes #567

@esc
Copy link
Member

esc commented Apr 1, 2020

@ARF1 thanks for submitting this, I have labeled it as needing review for now.

@esc esc added this to the Version 0.35.0 milestone Aug 18, 2020
@stuartarchibald stuartarchibald removed this from the Version 0.35.0 milestone Nov 6, 2020
@esc esc self-assigned this Mar 22, 2021
@gmarkall gmarkall added this to the PR Backlog milestone Dec 21, 2021
@esc esc removed their assignment Feb 16, 2022
gmarkall
gmarkall previously approved these changes Feb 6, 2023
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

I've merged main here and given this a test. It appears to do exactly as intended. For example now when running python setup.py clean we see:

$ python setup.py clean
/home/gmarkall/mambaforge/envs/numbadev/lib/python3.10/site-packages/setuptools/dist.py:493: UserWarning: Normalizing '0.40.0dev0+54.gab202ba' to '0.40.0.dev0+54.gab202ba'
  warnings.warn(tmpl.format(**locals()))
running clean
removing '/home/gmarkall/numbadev/llvmlite/llvmlite.egg-info' (and everything under it)
removing '/home/gmarkall/numbadev/llvmlite/ffi/build' (and everything under it)
removing '/home/gmarkall/numbadev/llvmlite/llvmlite/__pycache__' (and everything under it)
removing '/home/gmarkall/numbadev/llvmlite/__pycache__' (and everything under it)

ffi/build is removed.

This needs a quick second review as I made a change to the super() idiom used to call super().run() but otherwise I think this is good to go.

@gmarkall
Copy link
Member

gmarkall commented Feb 6, 2023

CI needs #903.

@esc
Copy link
Member

esc commented Feb 6, 2023

@gmarkall thank you for adopting this. When is the ffi/build directory created:

I did:

$ python setup.py build
...
$ git status --ignored
On branch wip/fix_setup_clean
Untracked files:
	ffi/libllvmlite.dylib.dSYM/
	gitlog2changelog.py
	test.sh

Ignored files:
	build/
	ffi/libllvmlite.dylib
	ffi/libllvmlite.dylib.dSYM/Contents/Resources/
	llvmlite/binding/libllvmlite.dylib
$ python setup.py clean
/Users/esc/miniconda3-arm64/envs/llvmlite_3.10/lib/python3.10/site-packages/setuptools/dist.py:529: UserWarning: Normalizing '0.40.0dev0+54.gab202ba385' to '0.40.0.dev0+54.gab202ba385'
  warnings.warn(tmpl.format(**locals()))
running clean
removing '/Users/esc/git/llvmlite/__pycache__' (and everything under it)
$ git status --ignored
On branch wip/fix_setup_clean
Untracked files:
	ffi/libllvmlite.dylib.dSYM/
	gitlog2changelog.py
	test.sh

Ignored files:
	build/
	ffi/libllvmlite.dylib
	ffi/libllvmlite.dylib.dSYM/Contents/Resources/
	llvmlite/binding/libllvmlite.dylib

So I am not yet sure when the clean command is supposed to pickup ffi/build? Can you explain?

@gmarkall
Copy link
Member

gmarkall commented Feb 6, 2023

It's created when using CMake to build. This happens on Windows, or if you build on Linux / macOS with the environment variable LLVMLITE_USE_CMAKE set to 1.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for continuing with this @gmarkall. I think there's a hard coded path separator that needs changing, then I guess the check as described above needs to be redone (the cmake related build on windows).

setup.py Outdated Show resolved Hide resolved
Co-authored-by: stuartarchibald <[email protected]>
@gmarkall
Copy link
Member

gmarkall commented Feb 6, 2023

@stuartarchibald thanks for catching that - now updated.

@gmarkall
Copy link
Member

gmarkall commented Feb 7, 2023

I also spotted that the clean command leaves .dll and .dylib files lying around. Testing the clean command on Windows:

(numbadev) PS C:\Users\gmarkall\work\numbadev\llvmlite> python setup.py clean
C:\Users\gmarkall\Miniconda3\envs\numbadev\lib\site-packages\setuptools\dist.py:487: UserWarning: Normalizing '0.40.0dev0+58.g8300243' to '0.40.0.dev0+58.g8300243'
  warnings.warn(tmpl.format(**locals()))
running clean
removing 'C:\Users\gmarkall\work\numbadev\llvmlite\llvmlite.egg-info' (and everything under it)
removing 'C:\Users\gmarkall\work\numbadev\llvmlite\llvmlite\binding\llvmlite.dll'
removing 'C:\Users\gmarkall\work\numbadev\llvmlite\ffi\build' (and everything under it)
removing 'C:\Users\gmarkall\work\numbadev\llvmlite\llvmlite\__pycache__' (and everything under it)
removing 'C:\Users\gmarkall\work\numbadev\llvmlite\__pycache__' (and everything under it)

After running the command, there is nothing left:

(numbadev) PS C:\Users\gmarkall\work\numbadev\llvmlite> git status --ignored
On branch wip/fix_setup_clean
Your branch is up to date with 'ARF1/wip/fix_setup_clean'.

nothing to commit, working tree clean

@gmarkall
Copy link
Member

gmarkall commented Feb 7, 2023

@stuartarchibald I think this should be ready for another look now.

Copy link
Member

@esc esc left a comment

Choose a reason for hiding this comment

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

I have checked the following:

 💣 zsh» git status --ignored
On branch wip/fix_setup_clean
Untracked files:
	gitlog2changelog.py
	test.sh

nothing added to commit but untracked files present

 💣 zsh» python setup.py build
...

 💣 zsh» git status --ignored
On branch wip/fix_setup_clean
Untracked files:
	ffi/libllvmlite.dylib.dSYM/
	gitlog2changelog.py
	test.sh

Ignored files:
	__pycache__/
	build/
	ffi/libllvmlite.dylib
	ffi/libllvmlite.dylib.dSYM/Contents/Resources/
	llvmlite/__pycache__/
	llvmlite/binding/libllvmlite.dylib

nothing added to commit but untracked files present

 💣 zsh» python setup.py clean
...                                                                                                                                                                          

 💣 zsh» git status --ignored
On branch wip/fix_setup_clean
Untracked files:
	ffi/libllvmlite.dylib.dSYM/
	gitlog2changelog.py
	test.sh

Ignored files:
	build/

nothing added to commit but untracked files present

So, only the build and ffi/libllvmlite.dylib.dSYM` remains. They contain:

 💣 zsh» tree   ffi/libllvmlite.dylib.dSYM/
ffi/libllvmlite.dylib.dSYM/
└── Contents
    ├── Info.plist
    └── Resources
        └── DWARF

3 directories, 1 file
 💣 zsh» tree   build
build
└── lib.macosx-11.1-arm64-cpython-310
    └── llvmlite
        ├── __init__.py
        ├── _version.py
        ├── binding
        │   ├── __init__.py
        │   ├── analysis.py
        │   ├── common.py
        │   ├── context.py
        │   ├── dylib.py
        │   ├── executionengine.py
        │   ├── ffi.py
        │   ├── initfini.py
        │   ├── linker.py
        │   ├── module.py
        │   ├── object_file.py
        │   ├── options.py
        │   ├── passmanagers.py
        │   ├── targets.py
        │   ├── transforms.py
        │   └── value.py
        ├── ir
        │   ├── __init__.py
        │   ├── _utils.py
        │   ├── builder.py
        │   ├── context.py
        │   ├── instructions.py
        │   ├── module.py
        │   ├── transforms.py
        │   ├── types.py
        │   └── values.py
        ├── tests
        │   ├── __init__.py
        │   ├── __main__.py
        │   ├── customize.py
        │   ├── refprune_proto.py
        │   ├── test_binding.py
        │   ├── test_ir.py
        │   ├── test_refprune.py
        │   ├── test_setup.py
        │   └── test_valuerepr.py
        └── utils.py

5 directories, 37 files

Would it be reasonable to assume that a clean command would remove those files also?

@esc
Copy link
Member

esc commented Jun 9, 2023

@gmarkall do you want to pickup work on this again? I think it got reasonably far and just needs a few more lines?

@gmarkall
Copy link
Member

gmarkall commented Jun 9, 2023

@esc I'm not able to pick this up again in the short term. If you're aiming to tidy up the PRs and reduce the number of things open, I don't mind if you'd prefer to close / abandon this.

@esc
Copy link
Member

esc commented Jun 28, 2023

@gmarkall it would be a shame to abandon this since it's fairly close to finish. I've placed it on the contributor self-service board, who knows, maybe we get lucky.

@esc esc removed this from the PR Backlog milestone Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Timeboxed Issue tasks upto 1 hour
Development

Successfully merging this pull request may close these issues.

python setup.py clean --all does not remove ffi/build
4 participants