Skip to content

BLD: separate lightpath and lightpath[gui] subpackages#190

Merged
tangkong merged 13 commits intopcdshub:masterfrom
tangkong:bld_lightpath_base
Aug 30, 2024
Merged

BLD: separate lightpath and lightpath[gui] subpackages#190
tangkong merged 13 commits intopcdshub:masterfrom
tangkong:bld_lightpath_base

Conversation

@tangkong
Copy link
Contributor

@tangkong tangkong commented Aug 26, 2024

Description

Separates lightpath base and gui-related dependencies

  • pip: lightpath and lightpath[gui]
  • conda: lightpath-base and lightpath (includes gui) I could not get conda builds working, we leave this split to the conda feedstock

Motivation and Context

Lightpath keeps dragging qt dependencies along with it

How Has This Been Tested?

pip installs locally. Conda eventually, but let's see if CI takes

Where Has This Been Documented?

This PR

@tangkong tangkong linked an issue Aug 26, 2024 that may be closed by this pull request
@tangkong tangkong force-pushed the bld_lightpath_base branch from 533a43e to 8331c69 Compare August 26, 2024 21:27
- name: lightpath-base
build:
noarch: python
script: python -m pip install . -vv
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
script: python -m pip install . -vv
script: {{ PYTHON }} -m pip install . -vv

I'll look for the docs on this

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/pcdshub/lightpath/actions/runs/10564732964/job/29267769359#step:12:49

Attempting to finalize metadata for lightpath-base
Error: Failed to render jinja template in /home/runner/work/lightpath/lightpath/conda-recipe/meta.yaml:
'PYTHON' is undefined

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this was the warning shot that suggests that something deeper was wrong with the conda build process

Copy link
Member

Choose a reason for hiding this comment

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

I see now that the recipes that do the *-base thing don't use {{ PYTHON }}, my bad for the red herring

@tangkong
Copy link
Contributor Author

After quite a bit of squinting at conda build logs, I've arrived at a point where the conda build step succeeds, but the test environment gets clobbered/lost and cannot be used for the rest of the test suite. This is despite the environment artifact being created and uploaded sensibly

The changes to the pip-side of things have worked from the onset, and I think are good moving forward. A nearly identical recipe works on the lightpath-feedstock and passes their build tests, so I think we'll revert this repo's recipe to the combined recipe and only split up the feedstock recipe.

@tangkong
Copy link
Contributor Author

adding --no-deps managed to remove the clobber warnings I think. There definitely was some double installations going on, I think what happens is that without --no-deps, the pip install installs all the package dependencies, then conda tries to follow the recipe and install them again.

We still lose the test_env folder though 😢

@ZLLentz
Copy link
Member

ZLLentz commented Aug 29, 2024

The remaining py311 failures come from pinning pytest<7.2.0, before this version it wasn't marked as noarch so the allowed versions are very pinned down

ZLLentz
ZLLentz previously approved these changes Aug 29, 2024
@tangkong
Copy link
Contributor Author

Ah we unpinned this a while ago. This was from an era where pytest-qt wasn't compatible

@ZLLentz
Copy link
Member

ZLLentz commented Aug 30, 2024

It's all GREEEN

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This is a great split that will help a lot in various cases!

@tangkong tangkong merged commit 8df784d into pcdshub:master Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider splitting into two independently installable packages

2 participants