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

Rename release targets and add linux-aarch64 #559

Merged
merged 7 commits into from
Sep 12, 2022

Conversation

IamTheFij
Copy link
Contributor

I'm unsure if this will work in the build workflow.

I'm unsure if this will work in the build workflow.
@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #559 (704f923) into main (e8fc691) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #559   +/-   ##
=======================================
  Coverage   97.82%   97.82%           
=======================================
  Files          14       14           
  Lines        5469     5469           
=======================================
  Hits         5350     5350           
  Misses        119      119           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@JohnnyMorganz
Copy link
Owner

Looks reasonable to me, but I will double check over the weekend

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Can you also modify https://github.com/JohnnyMorganz/StyLua/blob/main/stylua-npm-bin/binary.js to point to the aarch64 version for linux?

I think we also need to update the VSCode extension (https://github.com/JohnnyMorganz/StyLua/blob/main/stylua-vscode/src/util.ts#L18), but the vscode extension also needs fixing for macos aarch64

@IamTheFij
Copy link
Contributor Author

I could do that, yea. Overall, it would probably be easier to template these out if the formatting of the names was consistent. Eg. everything being stylua-{system}-{arch}.zip rather than having to optionally append the arch with a prefixed hyphen.

What do you think about that? Then I'd update this PR to overhaul the naming and update all downloaders (python and node) to use the new format.

@JohnnyMorganz
Copy link
Owner

JohnnyMorganz commented Sep 4, 2022

I could do that, yea. Overall, it would probably be easier to template these out if the formatting of the names was consistent. Eg. everything being stylua-{system}-{arch}.zip rather than having to optionally append the arch with a prefixed hyphen.

What do you think about that? Then I'd update this PR to overhaul the naming and update all downloaders (python and node) to use the new format.

Yeah in hindsight this is probably what should've happened. The only thing is there are a lot of consumers and changing how the artifact was named already caused headache the first time when we removed the version

@ViViDboarder
Copy link

Yea, is it better to just change now and get it over with though? Could probably have some period of time where the release maintains both files as well to make the transition easier.

@JohnnyMorganz
Copy link
Owner

Yeah ok, let's do it. We can append "-x86_64" to the end, and keep artifacts with the old name temporarily

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Could you also update the changelog so I don't forget about this when I create the next release?

@@ -89,6 +95,18 @@ jobs:
asset_name: ${{ matrix.artifact-name }}.zip
asset_content_type: application/zip

# TODO: Remove this after deprecation notice
- name: Upload Binary to Release aliases
if: ${{ matrix.artifact-alias == "" }}
Copy link
Owner

Choose a reason for hiding this comment

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

I think this condition should be negated?

Suggested change
if: ${{ matrix.artifact-alias == "" }}
if: ${{ matrix.artifact-alias != "" }}

@IamTheFij IamTheFij changed the title Add target for linux-aarch64 Rename release targets and add linux-aarch64 Sep 12, 2022
@IamTheFij
Copy link
Contributor Author

Ok. Fixed the condition, updated the pyproject to use the new formatting, and fixed the merge conflict by merging in master.

Looks like the lint for VSCode extension failed. I'm installing npm and prettier to fix it, but I'm also making a new branch to add pre-commit hooks to the repo to make it easier to catch things like this before pushing. So far, I'm just adding the same checks that already exist in GitHub Actions. That will be a different PR.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Thanks!

@JohnnyMorganz JohnnyMorganz merged commit 133393c into JohnnyMorganz:main Sep 12, 2022
@JohnnyMorganz
Copy link
Owner

Tried a dummy run and looks like it failed with an invalid workflow file: https://github.com/JohnnyMorganz/StyLua/actions/runs/3037707302/workflow

@JohnnyMorganz
Copy link
Owner

Fixed the syntax issue but looks like compiling linux aarch64 fails: https://github.com/JohnnyMorganz/StyLua/runs/8307187443?check_suite_focus=true

@IamTheFij
Copy link
Contributor Author

Addressed in #572

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.

None yet

3 participants