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

Symlink python packages #223

Closed

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 6, 2019

Fix #218

When using colcon build --symlink-install, install the packages with symlinks instead of using hooks to extend PYTHONPATH.

@rotu rotu mentioned this pull request Sep 11, 2019
@rotu rotu force-pushed the symlink-instead-of-pythonpath branch from c58a329 to 3207b54 Compare September 11, 2019 21:46
@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #223 into master will increase coverage by 2.28%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
+ Coverage   78.14%   80.43%   +2.28%     
==========================================
  Files          54       54              
  Lines        3080     3087       +7     
  Branches      507      507              
==========================================
+ Hits         2407     2483      +76     
+ Misses        646      570      -76     
- Partials       27       34       +7
Impacted Files Coverage Δ
colcon_core/task/python/build.py 31.12% <16.66%> (+31.12%) ⬆️
colcon_core/task/__init__.py 98.98% <0%> (+7.07%) ⬆️
colcon_core/task/python/__init__.py 23.8% <0%> (+9.52%) ⬆️
colcon_core/subprocess.py 64.75% <0%> (+16.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee5d9bc...5d4ace2. Read the comment docs.

@dirk-thomas
Copy link
Member

With #219 merged please rebase this one.

@rotu rotu force-pushed the symlink-instead-of-pythonpath branch from 8016577 to 0a7cf13 Compare September 13, 2019 18:49
@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) enhancement New feature or request labels Sep 13, 2019
@rotu
Copy link
Contributor Author

rotu commented Sep 13, 2019

Thanks for fixing the test failure. This PR is not functionally correct yet, so don't worry about its test status. I should probably add a test PR to accompany this enhancement too.

@rotu rotu force-pushed the symlink-instead-of-pythonpath branch from 533cd5d to 71d6beb Compare September 13, 2019 23:45
rotu and others added 5 commits September 19, 2019 17:36
Previously, we either monkey-patched setuptools and harvested the arguments passed to setuptools.setup or we parsed setup.cfg
Now we run the setup.py script with distutils.core.run_setup to return properties from the resulting Distribution object.
@rotu rotu force-pushed the symlink-instead-of-pythonpath branch 5 times, most recently from 8fb54ea to 264a054 Compare September 28, 2019 01:17
@rotu
Copy link
Contributor Author

rotu commented Sep 28, 2019

State of this PR:

  1. Packages now get their modules symlinked in instead of added via PYTHONPATH. This has the gigantic advantage that if two packages have identically named modules, you'll get a warning in symlink mode.
  2. Packages now log their installed files to install.log, like pip install.
  3. All tests are passing.

Things I don't love:

  1. I've reimplemented some of the logic of setup.py develop. This is less than ideal.
  2. This PR is a bit heavier than I had intended.
  3. I have too many PRs out. This should be on hold until that backlog comes down.

@rotu rotu force-pushed the symlink-instead-of-pythonpath branch from 264a054 to c042f29 Compare September 28, 2019 01:22
Modify build tests to build in symlink mode as well
Eggs was a bad idea. They still have to each be in PYTHONPATH to work.
Only install subdirectories of 
Log installed files w/ symlink_install
@rotu rotu force-pushed the symlink-instead-of-pythonpath branch from c042f29 to ac04854 Compare September 28, 2019 01:38
@dirk-thomas
Copy link
Member

@rotu Friendly ping to see if you are still interested in this?

@rotu
Copy link
Contributor Author

rotu commented Mar 7, 2020

Definitely still interested in seeing this done. But I haven't had the cycles to see it through yet. You can close this PR if you like

@dirk-thomas
Copy link
Member

I was just checking - it can stay open if you want to pick it up again at some point.

@dirk-thomas dirk-thomas marked this pull request as draft April 14, 2020 17:56
@dirk-thomas dirk-thomas changed the title WIP: Symlink python packages Symlink python packages Apr 14, 2020
@cottsay
Copy link
Member

cottsay commented Feb 3, 2024

Superseded by #592

@cottsay cottsay closed this Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress Actively being worked on (Kanban column)
Development

Successfully merging this pull request may close these issues.

Setup script produces a long PYTHONPATH with --symlink-install + --merge-install
4 participants