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

Reorganize repo to src-layout, migrate to hatch build system and pyproject.toml #1134

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
File renamed without changes.
26 changes: 26 additions & 0 deletions .github/workflows/check-release.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: Check Release
on:
push:
branches: [master, main]
pull_request:
branches:
- '*'

permissions:
contents: write

jobs:
check_release:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to step_build right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this workflow is for jupyter-releaser. Please look into its docs to have a better understanding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, other than the risk of bumping the version, why shouldn't it be in the step_build. The purpose for that one is to simulate a packaging step, which this one seems to do as well. In the publish.yml then the artifacts of pipx build and/or this job should be used.

From what I read in the implementation check-release is just a combination of all the other github actions, which in principle is more robust for using when releasing. What are your plans on how to implement this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly check-release simulates all the steps in actual jupyter-releaser workflow using mock GH instance. It does more than just building packages like simulating changelog generation, packaging, making release with mock instance, etc. As this workflow is very specific to jupyter-releaser, in my opinion it makes sense to keep it in a separate file. I mean, we have split the existing dense workflow file into logical step files for better readability. I think adding it to setp_build will again mishmash workflow files.

My original plan was to do include this check-release workflow simulation in the repo and give an idea to maintainers on what exactly it is doing. I guess @mwouts is taking of releases and if he wishes to use jupyter-releaser workflows for publishing packages, we can add the actual workflows in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

My view is that step_* workflows are not a single-job files, but more like logically separated ones. E.g. step_pre-commit should also have the npm pre-commits. In that sense it makes sense to have them in step_build (or renamed to something else) because in essence that step is about testing the packaging, and if this one includes changelog generation and all, all the better.

Main concern is the on: and linking the workflows one to another. But for this first-iteration we can keep it here since it still under consideration on how to implement.

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1
- name: Check Release
uses: jupyter-server/jupyter_releaser/.github/actions/check-release@v2
with:
token: ${{ secrets.GITHUB_TOKEN }}
Comment on lines +18 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one to be used before making a release? If so, move this one to publish.yml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, this workflow runs like any other workflows like continous-integration.yml and not necessarily before release. This makes quite a lot of checks like building and verifying python and node packages, etc. This workflow only makes sense if we want to migrate to jupyter-releaser and I have included it just to give an idea to the maintainers on what it does and to understand its internals.

- name: Upload Distributions
uses: actions/upload-artifact@v2
with:
name: jupyter-resource-usage-releaser-dist-${{ github.run_number }}
path: .jupyter_releaser_checkout/dist
34 changes: 34 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
name: CI
on:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add on: workflow_dispatch with configurable option of the build workflow

push:
paths-ignore:
- 'CHANGELOG.md'
branches: [main]
pull_request:
branches: [main]
schedule:
- cron: '0 11 * * 4'

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

jobs:
pre-commit:
uses: ./.github/workflows/step_pre-commit.yml

codeql:
needs: pre-commit
uses: ./.github/workflows/step_static-analysis.yml

test-pip:
needs: pre-commit
uses: ./.github/workflows/step_tests-pip.yml

test-conda:
needs: pre-commit
uses: ./.github/workflows/step_tests-conda.yml

build:
Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs: pre-commit

needs: [codeql, test-pip, test-conda]
uses: ./.github/workflows/step_build.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a re-actors/alls-green@release/v1 job to better visiualize experimental/required jobs. Here's an example:
https://github.com/swig/swig/pull/2647/files#diff-931bdf642388776529d5aeae22e23d9da62ffdadf2749c2d7aa7b4ce9c59b4a1R166-R174

181 changes: 0 additions & 181 deletions .github/workflows/continuous-integration.yml

This file was deleted.

17 changes: 11 additions & 6 deletions .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,26 @@ on:
jobs:
publish:
Copy link
Contributor

Choose a reason for hiding this comment

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

Add uses: ./.github/workflows/step_tests-pip.yml and etc. calls, calling all steps (can avoid static-analysis), with appropriate requires chaining.

runs-on: ubuntu-latest

environment:
name: pypi
url: https://pypi.org/p/jupytext

permissions:
id-token: write

steps:
- name: Checkout source
uses: actions/checkout@v3
- name: Set up Python 3.9
uses: actions/setup-python@v4
with:
python-version: 3.9

- name: Base Setup
uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1

- name: Build package
run: |
python -m pip install wheel jupyter-packaging 'jupyterlab>=3,<4'
BUILD_JUPYTERLAB_EXTENSION=1 python setup.py sdist bdist_wheel
python -m pip install wheel build
# Ensure that we build jupyterlab extension and distribute it with wheel
HATCH_BUILD_HOOKS_ENABLE=true python -m build

- name: Publish
uses: pypa/gh-action-pypi-publish@release/v1
66 changes: 66 additions & 0 deletions .github/workflows/step_build.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
name: build
run-name: Build test

on:
workflow_call:

concurrency:
group: ${{ github.ref }}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean about concurrency deadlock? What issues have you encountered?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. Never realized I had such configuration on my project. On the swig project I've put it that only the top-level has concurrency. Setting the concurrency to github.ref has the potential of cancelling running jobs when triggered manually or something (don't remember the context of why I've changed that).

cancel-in-progress: true

jobs:
build:
runs-on: ubuntu-latest
steps:
- name: Checkout source
uses: actions/checkout@v3

- name: Base Setup
uses: jupyterlab/maintainer-tools/.github/actions/base-setup@v1

- name: Lint the extension
run: |
# Install JupyterLab in an isolated env to get jlpm
pipx install jupyterlab>=4
pipx list

# Install lint deps and lint extension
cd jupyterlab/
jlpm
jlpm run lint:check
cd ..

# Uninstall pipx env
pipx uninstall jupyterlab

- name: Build package
run: |
python -m pip install build wheel

# Build jupytext package
HATCH_BUILD_HOOKS_ENABLE=true python -m build

# Don't publish a tar.gz file over 1MB (Issue #730)
if (($(wc -c < dist/*.tar.gz) > 1000000)); then exit 1; fi

# node_modules should not be in the package
if (($(tar -tf dist/*.tar.gz | grep node_modules | wc -l)>0)); then echo "node_modules should not be included" && exit 1; fi

# Check that the lab is there
if (($(tar -tf dist/*.tar.gz | grep jupyterlab/packages/jupyterlab_jupytext/labextension/package.json$ | wc -l)==0)); then echo "Missing lab extension" && exit 1; fi

# Install package and extension
python -m pip install dist/*.tar.gz

# Check extension
jupyter labextension list
jupyter labextension list 2>&1 | grep -ie "jupyterlab-jupytext.*OK"
python -m jupyterlab.browser_check

echo "Install went OK"

- name: Archive build artifacts
uses: actions/upload-artifact@v3
with:
name: dist
path: dist
Comment on lines +62 to +66
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this step conditional on workflow_call.inputs. For usual CI runs we don't need it, but on manual runs and CD we need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would say this is the build test and I think it would be a good idea to run them automatically on each PR. We dont want PRs to break builds, especially when labextension is involved.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I wasn't clear here. I meant only the step upload-artifact. It just a small QOL improvement to not have too many artifacts to search through.

20 changes: 20 additions & 0 deletions .github/workflows/step_pre-commit.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: pre-commit
run-name: Pre-commit

on:
workflow_call:

concurrency:
group: ${{ github.ref }}
cancel-in-progress: true

jobs:
pre-commit:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: "3.x"
- uses: pre-commit/[email protected]
26 changes: 26 additions & 0 deletions .github/workflows/step_static-analysis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
name: static-analysis
run-name: Run CodeQL analysis

on:
workflow_call:

Copy link
Contributor

Choose a reason for hiding this comment

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

This one needs more specific permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the permissions are inside jobs stanza.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok, I re-read the documentation on the permissions, you are right, that is a more valid approach. I was afraid that it was doing a merge operation where the global ones are still initialized.

concurrency:
group: ${{ github.ref }}
cancel-in-progress: true

jobs:
codeql:
runs-on: ubuntu-latest
permissions:
security-events: write
steps:
- name: Checkout repository
uses: actions/checkout@v3

- name: Initialize CodeQL
uses: github/codeql-action/init@v2
with:
languages: python, javascript

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@v2
Loading