-
Notifications
You must be signed in to change notification settings - Fork 11
Add swig dependency #2
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 swig dependency #2
Conversation
867ba8a
to
bf8b1af
Compare
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.
This needs to land in dtc upstream:
https://github.com/dgibson/dtc
But setup.py is a bit out-of-sync in the two projects, so I will see if I can tidy that up.
setup.py
Outdated
cmdclass = {'build_py' : build_py}, | ||
use_scm_version=True, | ||
setup_requires = ['setuptools_scm'], | ||
setup_requires = ['setuptools', 'swig'], |
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.
Do you need the change to setuptools ?
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.
AIUI, 'swig' here is just a wrapper package to download and install swig which is not a python package. What happens if the distro version of swig is already installed?
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.
I use python 3.12 to verify it on Windows, setuptools is necessary.
Without this PR and don't install swig manually
- py -3.12 -m pip install git+https://github.com/devicetree-org/pylibfdt.git@main
With this PR and don't install swig manually.
- py -3.12 -m pip install git+https://github.com/gguo11837463/pylibfdt.git@gua_pylibfdt_depend_swig
Good luck with that. I gave up. The issue is that python has its own ideas about how to build things and that's at odds with make and meson (more meson). IMO, we shouldn't try to wrap the python build with make and meson, but let users interact with python build directly that anyone familiar with python packages understands (i.e. running ./setup.py or pip). And it's not just running the build/install, but all the other python tools which integrate in such as pytest. Or at a minimum, finish the meson conversion first, so there's not 2 environments you are trying to shoehorn setup.py into. Also, I'm sure you are aware, but setup.py has been superseded by setup.cfg^Wpyproject.toml. Might want to consider moving to that before wrestling with make/meson integration. I'm sure pyproject.toml has its own quirks to deal with. The other issue was also what is the source of libfdt. Is it the one you build (and install?) or the distro installed one. IIRC, that was at odds with what I wanted to do here. |
7cbe824
to
e5f1dad
Compare
I just use it https://pypi.org/project/pylibfdt/ by pip install pylibfdt, but if I don't install by chocolatey install swig or https://sourceforge.net/p/swig/news/2024/02/swig-421-released/ on windows, pip install always failure, so want to make it done by this way. |
We don't want to download swig manually. Hope pip install can finish it directly. Signed-off-by: Gua Guo <gua.guo@intel.com>
e5f1dad
to
8121f99
Compare
I sent some patches upstream too |
Windows apparently builds pylibfdt if swig is declared as a dependency in the setup file. Otherwise it must be installed manually and Windows does not sport a package manager, so this is hard for users. This change seems to have no ill effects on Linux, at least. So add a new setup_requires line. Commit-notes: This was picked up from: devicetree-org/pylibfdt#2 END Series-to: dtc Series-cc: Gua Guo <gua.guo@intel.com>, david Signed-off-by: Gua Guo <gua.guo@intel.com> Signed-off-by: Simon Glass <sjg@chromium.org>
We don't want to download swig manually.
Hope pip install can finish it directly.