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

Allow overriding --prefix in spin build #241

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Oct 16, 2024

Fixes #240

This lets users override --prefix. Right now it's hard-coded, which leads to breakage on windows sometimes.

@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Oct 16, 2024

@stefanv I think your existing windows tests do run on powershell. You can see what shell an actions step is running by expanding the input details.

image

So I think the issue isn't powershell so much as how python is set up. I can trigger the problem using a python installed via micromamba and uv on Windows.

@stefanv
Copy link
Member

stefanv commented Oct 16, 2024

We don't actually install to /usr, though, that is the prefix used under the build-install dir. I think that is in place to make some of the checking work, by having the path names be predictable.

The tests seem happy, though, so perhaps you're right!

@stefanv stefanv added the type: Bug fix Something isn't working label Oct 16, 2024
@stefanv
Copy link
Member

stefanv commented Oct 16, 2024

(It would be good to trigger a Windows failure in the test suite, so that we don't break this again in the future by accident.)

@stefanv
Copy link
Member

stefanv commented Oct 16, 2024

python installed via micromamba and uv on Windows

This is very helpful; I could make a matrix entry for that.

@ngoldbaum
Copy link
Contributor Author

For uv you can do something like this on github actions:

    - uses: astral-sh/setup-uv@v3
    - run: |
        uv python install ${{ matrix.version }}
        uv venv --python ${{ matrix.version }}
    - run: |
        .venv\Scripts\activate
        uv pip install some_dependency
        python -m pytest

@ngoldbaum
Copy link
Contributor Author

Darn, it looks like simply installing python from uv wasn't enough to trigger it in the spin tests. I'm not sure how to write a test using the existing framework to trigger what I'm seeing with NumPy.

I guess I could add a CI job that just builds NumPy?

I'll leave it up to you, please feel free to push to this PR if you want to adjust things.

@stefanv
Copy link
Member

stefanv commented Oct 17, 2024

Thank you so much, Nathan. I'm out until next week, but will take a look at it then.

@ngoldbaum ngoldbaum changed the title do not set --prefix by default Allow overriding --prefix in spin build Oct 18, 2024
@ngoldbaum
Copy link
Contributor Author

OK, I've added a new --prefix option to meson.build. If I update NumPy's build command to match meson's everything works and I'm able to use spin build --prefix with numpy. Also added a test.

I also ran into #242, I think it's too easy for commands that projects want to override to go out of sync with upstream improvements.

@ngoldbaum
Copy link
Contributor Author

I also just checked on the Windows system I originally had a problem on - changing the default prefix to "C:\\" allows me to build on Windows.

@ngoldbaum
Copy link
Contributor Author

Hi, any chance we can get this reviewed? I tried again today to set up Windows tests for NumPy using spin on the free-threaded build using an alternate way to install Python and I hit this issue again, so setting up tests on Windows is currently blocked on this.

@stefanv stefanv merged commit 2a3dc50 into scientific-python:main Nov 5, 2024
20 checks passed
@stefanv
Copy link
Member

stefanv commented Nov 5, 2024

Thanks @ngoldbaum, and sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Bug fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"spin build" errors on windows
3 participants