jorisvandenbossche commented on code in PR #353:
URL: https://github.com/apache/arrow-nanoarrow/pull/353#discussion_r1447360090


##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3
+    - name: Check that cmake is installed
+      run: |
+        cmake --version
+
+    - name: Install build
+      run: |
+        pip install build
+
+    - name: Build sdist
+      run: |
+        cd python
+        python -m build --sdist
+
+    - name: Check install from sdist
+      run: |
+        pip install python/dist/nanoarrow-*.tar.gz
+
+    - name: Test import
+      run: |
+        python -c "import nanoarrow; print(nanoarrow.__version__)"
+
+    - uses: actions/upload-artifact@v3

Review Comment:
   ```suggestion
       - uses: actions/upload-artifact@v4
   ```



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3

Review Comment:
   ```suggestion
       - uses: actions/setup-python@v5
   ```



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3
+    - name: Check that cmake is installed
+      run: |
+        cmake --version
+
+    - name: Install build
+      run: |
+        pip install build
+
+    - name: Build sdist
+      run: |
+        cd python
+        python -m build --sdist
+
+    - name: Check install from sdist
+      run: |
+        pip install python/dist/nanoarrow-*.tar.gz
+
+    - name: Test import
+      run: |
+        python -c "import nanoarrow; print(nanoarrow.__version__)"
+
+    - uses: actions/upload-artifact@v3
+      with:
+        name: sdist
+        path: ./python/dist/nanoarrow-*.tar.gz
+
+  build_wheels:
+    needs: ["build_sdist"]
+    name: Build wheels on ${{ matrix.config.label }}
+    runs-on: ${{ matrix.config.os }}
+    strategy:
+      matrix:
+        config:
+          - {os: "ubuntu-20.04", label: "linux"}
+          - {os: "windows-2019", label: "windows"}
+          - {os: "macOS-11", label: "macOS"}
+          # Uncomment when apache/arrow-nanoarrow is added to the arrow runner 
group
+          # - {os: ["self-hosted", "arm"], label: "linux/arm64"}
+
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v3
+
+      - name: Install cibuildwheel
+        run: python -m pip install cibuildwheel==2.15.0
+
+      - name: Build wheels
+        run: |
+          python -m cibuildwheel --output-dir wheelhouse python

Review Comment:
   ```suggestion
         - name: Build wheels
           uses: pypa/[email protected]
   ```
   
   I am not sure why the cibuildwheel README shows the manual way, but the docs 
recommend using the action 
(https://cibuildwheel.readthedocs.io/en/stable/setup/#github-actions)



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3
+    - name: Check that cmake is installed
+      run: |
+        cmake --version
+
+    - name: Install build
+      run: |
+        pip install build
+
+    - name: Build sdist
+      run: |
+        cd python
+        python -m build --sdist
+
+    - name: Check install from sdist
+      run: |
+        pip install python/dist/nanoarrow-*.tar.gz
+
+    - name: Test import
+      run: |
+        python -c "import nanoarrow; print(nanoarrow.__version__)"
+
+    - uses: actions/upload-artifact@v3
+      with:
+        name: sdist
+        path: ./python/dist/nanoarrow-*.tar.gz
+
+  build_wheels:
+    needs: ["build_sdist"]
+    name: Build wheels on ${{ matrix.config.label }}
+    runs-on: ${{ matrix.config.os }}
+    strategy:
+      matrix:
+        config:
+          - {os: "ubuntu-20.04", label: "linux"}
+          - {os: "windows-2019", label: "windows"}
+          - {os: "macOS-11", label: "macOS"}
+          # Uncomment when apache/arrow-nanoarrow is added to the arrow runner 
group
+          # - {os: ["self-hosted", "arm"], label: "linux/arm64"}
+
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v3
+
+      - name: Install cibuildwheel
+        run: python -m pip install cibuildwheel==2.15.0
+
+      - name: Build wheels
+        run: |
+          python -m cibuildwheel --output-dir wheelhouse python
+        env:
+          CIBW_ARCHS_MACOS: x86_64 arm64
+          # Optional (test suite will pass if these are not available)
+          # Commenting this for now because not all the tests pass yet (fixes 
in another PR)
+          # CIBW_BEFORE_TEST: pip install --only-binary ":all:" pyarrow numpy 
|| pip install --only-binary ":all:" numpy || true
+          CIBW_TEST_REQUIRES: pytest
+          CIBW_TEST_COMMAND: pytest {package}/tests
+
+      - uses: actions/upload-artifact@v3
+        with:
+          name: wheels

Review Comment:
   ```suggestion
         - uses: actions/upload-artifact@v4
           with:
             name: wheels-${{ matrix.config.label }}
   ```
   
   (the latest version requires unique names)



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3
+    - name: Check that cmake is installed
+      run: |
+        cmake --version
+
+    - name: Install build
+      run: |
+        pip install build
+
+    - name: Build sdist
+      run: |
+        cd python
+        python -m build --sdist
+
+    - name: Check install from sdist
+      run: |
+        pip install python/dist/nanoarrow-*.tar.gz
+
+    - name: Test import
+      run: |
+        python -c "import nanoarrow; print(nanoarrow.__version__)"
+
+    - uses: actions/upload-artifact@v3
+      with:
+        name: sdist
+        path: ./python/dist/nanoarrow-*.tar.gz
+
+  build_wheels:
+    needs: ["build_sdist"]
+    name: Build wheels on ${{ matrix.config.label }}
+    runs-on: ${{ matrix.config.os }}
+    strategy:
+      matrix:
+        config:
+          - {os: "ubuntu-20.04", label: "linux"}
+          - {os: "windows-2019", label: "windows"}
+          - {os: "macOS-11", label: "macOS"}
+          # Uncomment when apache/arrow-nanoarrow is added to the arrow runner 
group
+          # - {os: ["self-hosted", "arm"], label: "linux/arm64"}

Review Comment:
   BTW, we can build MacOS arm64 wheels here on github actions if we want (just 
not test them), that's what we do in other projects I am involved with



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:

Review Comment:
   We could also run in on every commit in main? (I agree it's certainly good 
to not let it run on every commit in every PR)
   
   ```
     push:
       branches:
         - main
   ```



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'

Review Comment:
   ```suggestion
         - '.github/workflows/python-wheels.yaml'
         - 'python/setup.py'
         - 'python/pyproject.toml'
         - 'python/bootstrap.py'
         - 'python/MANIFEST.in'
   ```



##########
.github/workflows/python-wheels.yaml:
##########
@@ -0,0 +1,97 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: Build Python Wheels
+
+# Build wheels weekly or when requested
+on:
+  pull_request:
+    branches:
+      - main
+    paths:
+      - '.github/workflows/python-wheels.yaml'
+  workflow_dispatch:
+  schedule:
+    - cron: '6 0 * * 0'
+
+jobs:
+  build_sdist:
+    runs-on: "ubuntu-20.04"
+    steps:
+    - uses: actions/checkout@v4
+    - uses: actions/setup-python@v3
+    - name: Check that cmake is installed
+      run: |
+        cmake --version
+
+    - name: Install build
+      run: |
+        pip install build
+
+    - name: Build sdist
+      run: |
+        cd python
+        python -m build --sdist
+
+    - name: Check install from sdist
+      run: |
+        pip install python/dist/nanoarrow-*.tar.gz

Review Comment:
   As an additional check, we could also run `twine check --strict 
python/dist/*`



##########
python/pyproject.toml:
##########
@@ -34,7 +34,7 @@ repository = "https://github.com/apache/arrow-nanoarrow";
 [build-system]
 requires = [
     "setuptools >= 61.0.0",
-    "setuptools-scm",
+    "versioneer[toml]==0.29",

Review Comment:
   This isn't needed if you use the copied miniver helper?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to