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

PR: Install wheel in macOS build environment #20835

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Apr 17, 2023

Description of Changes

Install wheel in macOS standalone build environment

Issue(s) Resolved

Fixes #20834

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @mrclary

@mrclary mrclary self-assigned this Apr 17, 2023
@mrclary mrclary marked this pull request as draft April 17, 2023 05:58
@mrclary mrclary changed the base branch from master to 5.x April 17, 2023 05:58
@mrclary mrclary marked this pull request as ready for review April 17, 2023 06:08
@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

Installing wheel seemed to have fixed the issue, but I don't understand why. Perhaps @CAM-Gerlach has some insight?

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

FYI, @spyder-ide/core-developers, after removing the macOS and Windows standalone installer workflows from the master branch (since the will not be used for 6.x), the workflow_dispatch (manual trigger) no longer works. This is because workflow_dispatch must be present in the workflow in the master branch in order for it to be available for any other branch. This was an oversight of mine when submitting #20368.

We may want to discuss whether we want to retain some dummy workflows in the master branch just for this purpose, at least until development on 5.x is completely terminated.

@ccordoba12
Copy link
Member

the workflow_dispatch (manual trigger) no longer works

I think @dalthviz found a way to work around that.

@CAM-Gerlach
Copy link
Member

We may want to discuss whether we want to retain some dummy workflows in the master branch just for this purpose, at least until development on 5.x is completely terminated.

You could just remove all the triggers but workflow_dispatch from the workflow in master?

I think @dalthviz found a way to work around that.

Oh what was that?

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

the workflow_dispatch (manual trigger) no longer works

I think @dalthviz found a way to work around that.

@dalthviz do tell!

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

You could just remove all the triggers but workflow_dispatch from the workflow in master?

installer-macos.yml and installer-win.yml no longer exist on the master branch; that is why the workflow_dispatch trigger does not work even though the workflows exist on 5.x.

https://docs.github.com/en/actions/managing-workflow-runs/manually-running-a-workflow#configuring-a-workflow-to-run-manually

"To trigger the workflow_dispatch event, your workflow must be in the default branch."

@CAM-Gerlach
Copy link
Member

installer-macos.yml and installer-win.yml no longer exist on the master branch; that is why the workflow_dispatch trigger does not work even though the workflows exist on 5.x.

Right—what I'm saying is in master, add them back but with only the workflow_dispatch trigger

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

Right—what I'm saying is in master, add them back but with only the workflow_dispatch trigger

Oh, sorry for the misunderstanding. Yes, I agree, this is what I meant by a dummy workflow.

@CAM-Gerlach
Copy link
Member

It seems we are in violent agreement, then :)

Installing wheel seemed to have fixed the issue, but I don't understand why. Perhaps @CAM-Gerlach has some insight?

Hmm, well the proximate issue appears to be that without wheel in the local env, in the Install Dependencies step pip/Setuptools tries a PEP 660 isolated editable build and install relying on the pyproject.toml metadata, which we don't have (yet):

Obtaining file:///Users/runner/work/spyder/spyder
  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Checking if build backend supports build_editable: started
  Checking if build backend supports build_editable: finished with status 'done'
  Getting requirements to build editable: started
  Getting requirements to build editable: finished with status 'done'
  Preparing editable metadata (pyproject.toml): started
  Preparing editable metadata (pyproject.toml): finished with status 'done'

With wheel it does what appears to be a non-isolated build, gets the metadata from the setup.py (where it is) and may use the legacy editable install method instead (not clear):

Obtaining file:///Users/runner/work/spyder/spyder
  Preparing metadata (setup.py): started
  Preparing metadata (setup.py): finished with status 'done'

I would have to dig a lot deeper (or ask the pip or Setuptools folks I know — the frontend-backend boundary here isn't totally clear from the available output) to figure out why exactly that is. Once I finally get to upgrading the packaging metadata for this repo, I can take a closer look for it seems this solution works for now.


Also, just FYI, in the Determine build matrix job, you need to change the line

echo "::set-output name=build_type::{'build_type':$(echo $build_type)}"

to

echo "name=build_type::{'build_type':$(echo $build_type)}" >> $GITHUB_OUTPUT

to avoid the warning (which will become an error in a month and a half:

Warning: The `set-output` command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

per this changelog entry.

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

Hmm, well the proximate issue appears to be that without wheel in the local env, in the Install Dependencies step pip/Setuptools tries a PEP 660 isolated editable build and install relying on the pyproject.toml metadata, which we don't have (yet):

I would have to dig a lot deeper (or ask the pip or Setuptools folks I know — the frontend-backend boundary here isn't totally clear from the available output) to figure out why exactly that is. Once I finally get to upgrading the packaging metadata for this repo, I can take a closer look for it seems this solution works for now.

I figured it had something to do with that black magic stuff that I don't understand. I just think it's odd that the behavior would change when none of the inputs have changed...

Also, just FYI, in the Determine build matrix job, you need to change the line

Yes, I've started incorporating that into all my other workflows and just haven't updated it here yet.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @mrclary!

@mrclary
Copy link
Contributor Author

mrclary commented Apr 17, 2023

@CAM-Gerlach, do you want to review this or are we okay?

@dalthviz
Copy link
Member

Just in case, when doing the pre release steps for Spyder 5.4.3 I was able to run the workflows related with the installers by using the GitHub CLI, I did notice that the button to trigger the workflows was unavailable from the GitHub web interface but maybe using the GitHub CLI workarounds that since you can provide a ref from where to run the workflow? 🤔

Anyhow, I described the usage of the GitHub CLI for running the workflows here: https://github.com/spyder-ide/spyder/blob/5.x/RELEASE.md#check-release-candidate

@mrclary
Copy link
Contributor Author

mrclary commented Apr 18, 2023

Okay, I'll just add some dummy workflows back into master in another PR.

@ccordoba12
Copy link
Member

are we okay?

I think we're okay. This is a relatively simple change which will fix our CIs in 5.x, so let's merge it.

@ccordoba12 ccordoba12 merged commit 00ebc4a into spyder-ide:5.x Apr 18, 2023
ccordoba12 added a commit that referenced this pull request Apr 18, 2023
@mrclary mrclary deleted the issue-20834 branch April 18, 2023 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spyder standalone bundle workflow fails
4 participants