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

build+ci: make local, gh, and Jenkins envs identical #2019

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

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Aug 9, 2024

Description

An attempt to get our Python environments consistent across: local, Github CI, and Jenkins builds.

Changelog

  • CI would just upgrade pip to the latest sometimes. make venv did not upgrade pip. Jenkins pinned pip to 23.0.1. To make this consistent, CI now calls make venv and make venv installs pip==23.0.1.
  • CI installed pydocstyle pylint pytest separately from calling make install. pylint and pytest were already listed in every indicator's setup.py and pydocstyle was haphardly listed, so I removed the install command from CI, and added pydocstyle to the rest of the indicators.
  • Jenkins had an unnecessary pip install numpy command since each indicator either explicitly lists it as a dependency or transitively through Pandas, so I removed that line.
  • twine and build are needed to build and publish _delphi_utils and were only installed in the release CI. Added that to the [dev] section of _delphi_utils dependencies instead and removed the CI-specific line.
  • wheel had a separate install in many places, but we only need it for the bdist_wheel command, which setuptools>=70.1 provides (see historical note), so I removed it and added the setuptools>=70.1 version requirement to _delphi_utils build environment.

Associated Issue(s)

@dshemetov dshemetov changed the base branch from 1973-clean-up-delphi-utils-take-2 to main August 9, 2024 17:05
@dshemetov dshemetov force-pushed the ds/clean-clean-delphi-utils branch from 92ee419 to cfb451b Compare August 9, 2024 17:14
* pin pip==23.0.1 everywhere
* remove pip install calls from ci and deploy,
  make sure all dependencies are listed in
  pyproject.toml or setup.py and installed by the
  Makefile
* remove wheel from dependencies, only needed
  wheel for bdist_wheel command, which
  setuptools>=70.1 provides
@dshemetov dshemetov force-pushed the ds/clean-clean-delphi-utils branch from 983454b to 8a72ca0 Compare August 9, 2024 17:36
@dshemetov dshemetov changed the base branch from main to 1973-clean-up-delphi-utils-take-2 August 9, 2024 18:03
@dshemetov dshemetov changed the title build+ci: make dev envs identical with gh build envs build+ci: make local, gh, and Jenkins envs identical Aug 9, 2024
Base automatically changed from 1973-clean-up-delphi-utils-take-2 to main December 10, 2024 22:23
@aysim319
Copy link
Contributor

need to add changes nhsn since nhsn was added in the new release

"Development Status :: 5 - Production/Stable",
"Intended Audience :: Developers",
"Programming Language :: Python :: 3.8",
"License :: MIT",
Copy link
Contributor

@aysim319 aysim319 Dec 17, 2024

Choose a reason for hiding this comment

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

need to change this to "License :: OSI Approved :: MIT License"; fixing the merge conflict should fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should keep this... I know previously we wanted to convert all the setup.py into pyproject.toml, but I rememeber the release where we converted delphi_utils setup.py into pyproject.toml has some hiccups....

Copy link
Contributor

Choose a reason for hiding this comment

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

I had some issues using the pyproject.toml for nhsn and ended up just using the setup.py. since this codebase will be replaced, I think it's better to keep setup.py and have it consistent with the other indicators

@aysim319 aysim319 requested a review from minhkhul December 17, 2024 20:03
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.

2 participants