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

Use normal python build workflow instead of in-tree builds #558

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented May 22, 2022

The project currently uses a rather unconventional in-tree build workflow (I'd never encountered it before in several years of python distro packaging), and I haven't yet been able to find a convincing reason as to why. Of course it's very possible I'm just missing something, but in case I'm not, here's a PR that converts it to the standard python build workflow.

@umarcor
Copy link
Contributor

umarcor commented May 22, 2022

Hi @Xiretza! Nice to see you around!

Overall, I do agree. However, note the deprecation warning that is shown if the in-tree is not used:

https://github.com/chipsalliance/f4pga/runs/6543658873?check_suite_focus=true#step:4:11

 Processing /home/runner/work/f4pga/f4pga
  DEPRECATION: A future pip version will change local packages to be built in-place without first copying to a temporary directory. We recommend you use --use-feature=in-tree-build to test your packages with this new behavior before it becomes the default.
   pip 21.3 will remove support for this functionality. You can find discussion regarding this at https://github.com/pypa/pip/issues/7555.

pypa/pip#7555

So, I don't have an strong opinion in favour of one solution or the other. I'd like to understand whether not using in-tree now might produce a regression in the near future.

@Xiretza
Copy link
Contributor Author

Xiretza commented May 22, 2022

Ah, yes, I had misinterpreted that flag - it's actually unrelated to putting setup.py in the module directory, it's just an opt-in for behaviour that will become the default in the future. I've added it back in.

@Xiretza
Copy link
Contributor Author

Xiretza commented May 22, 2022

Hm, the eos-s3 family somehow installs an older pip version - not sure how that can happen, but I've removed the --use-feature=in-tree-build option from that test job.

@umarcor
Copy link
Contributor

umarcor commented May 22, 2022

In the end, this PR is removing package_dir={"f4pga": "."}, from the setup.py, and moving both setup.py and requirements.txt to the root of the repo. I guess that will require changing https://github.com/SymbiFlow/f4pga-arch-defs/blob/main/conda_lock.yml#L106 to remove the "subdirectory" option. I'm not sure we want to do that. Subdir f4pga is "standalone" now (it can be "moved" anywhere). By moving the setup to the root, we make the "whole" repo part of the pip package. Overall, when this package is published in pip, users will be unaware of the actual location of the sources within the repo.

The QuickLogic environment is expected to be brought up to date, on par with the xc7 environment. Furthermore, the conflicts between arch-defs packages are to be solved, so that assets for both architecture families can be installed side-to-side. Hence, we might want to keep this PR open for the original purpose (removing option in-tree) when the environments are updated and pip is bumped.

@umarcor umarcor added Enhancement New feature or request CI: Github Actions Continuous Integration issues related to GitHub Actions (Windows, Linux and MacOS) f4pga (python) Python package Discussion labels Jun 14, 2022
@umarcor
Copy link
Contributor

umarcor commented Jul 31, 2022

I'm closing this since it was implemented in 9308536.

@umarcor umarcor closed this Jul 31, 2022
@Xiretza
Copy link
Contributor Author

Xiretza commented Jul 31, 2022

That commit does the exact opposite of this PR.

@umarcor umarcor reopened this Jul 31, 2022
@umarcor
Copy link
Contributor

umarcor commented Jul 31, 2022

@Xiretza the commit removes --use-feature=in-tree-build, isn't it?

@Xiretza
Copy link
Contributor Author

Xiretza commented Jul 31, 2022

It does, and this PR does not (as far as I can tell, --use-feature=in-tree-build is a desired option for future-proofing). The PR is about building the python package from the repository root (the normal python build workflow) instead of having to go into the f4pga source directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Github Actions Continuous Integration issues related to GitHub Actions (Windows, Linux and MacOS) Discussion Enhancement New feature or request f4pga (python) Python package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants