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

code-cursor: add aarch64-linux #373536

Merged
merged 1 commit into from
Jan 14, 2025
Merged

Conversation

sarahec
Copy link
Contributor

@sarahec sarahec commented Jan 13, 2025

Changes

  1. Added aarch64-linux to supported platforms
  2. Altered update.sh to collect new platform
  3. Refactored the AppImage packaging step into a function
  4. Refactored the AppImage install step into a function

This will facilitate adding Darwin in #373494

Tests

  1. Confirmed update.sh works for both linux platforms
  2. Confirmed that x86_64-linux builds the same binary as before the change

For reviewers

  1. Please confirm that aarch64-linux builds. Note: This is an unfree package.
  2. The unusual structure (defining the input source to the derivation and the install phase in the upper let block) is so we can add Darwin installation easily. Darwin uses a different file format (.dmg disk images) and will need its own repack and install phases. We can assign these conditionally to src and installPhase based on platform class.

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 13, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 13, 2025
@sarahec sarahec changed the title code-cursor: prepare for Darwin code-cursor: add aarch64-linux, prepare for adding Darwin Jan 13, 2025
@sarahec sarahec requested a review from GaetanLepage January 13, 2025 20:09
@sarahec
Copy link
Contributor Author

sarahec commented Jan 13, 2025

@GaetanLepage I believe you have the ability to build aarch64-linux? If so, I welcome a nixpkgs-review and a PR review.

@sarahec sarahec added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jan 13, 2025
Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

Suggestion.

pkgs/by-name/co/code-cursor/package.nix Show resolved Hide resolved
pkgs/by-name/co/code-cursor/package.nix Show resolved Hide resolved
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373536


aarch64-linux

✅ 1 package built:
  • code-cursor

@sarahec sarahec mentioned this pull request Jan 13, 2025
13 tasks
Copy link
Contributor

@aspauldingcode aspauldingcode left a comment

Choose a reason for hiding this comment

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

Exciting to see. I think this looks good.

@sarahec
Copy link
Contributor Author

sarahec commented Jan 13, 2025

Exciting to see. I think this looks good.

Thank you. I just need to work out a few stylistic questions with Gaetan, who is reviewing.

Edit: I think the main issue was he wanted to replace the table of two URLs with a single URL with a conditional part and a list of hashes. Unfortunately, it doesn't quite do what we want.

@sarahec sarahec force-pushed the push-zmxovnqnpvsn branch 2 times, most recently from 18dfc27 to 3e0982c Compare January 13, 2025 21:12
@sarahec sarahec requested a review from GaetanLepage January 13, 2025 21:14
@aspauldingcode
Copy link
Contributor

Edit: I think the main issue was he wanted to replace the table of two URLs with a single URL with a conditional part and a list of hashes. Unfortunately, it doesn't quite do what we want.

I think that this is because Cursor's Download links for macOS has spaces in them, for whatever reason. Makes it really hard to find the hashes too.

It could've been a single url, passing variables through the download link based on the OS, architecture, and version of the package which would simplify things further, but I couldn't figure out the %20's that are added in the url

pkgs/by-name/co/code-cursor/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/co/code-cursor/package.nix Outdated Show resolved Hide resolved
@sarahec
Copy link
Contributor Author

sarahec commented Jan 14, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373536


x86_64-linux

@sarahec sarahec changed the title code-cursor: add aarch64-linux, prepare for adding Darwin code-cursor: add aarch64-linux Jan 14, 2025
@sarahec
Copy link
Contributor Author

sarahec commented Jan 14, 2025

Changed the update script to be less sensitive to filename changes. It now extracts everything up to the architecture ID then constructs URLs for x86/arm64. This pattern will make Darwin support easier.

@bet4it
Copy link

bet4it commented Jan 14, 2025

Screenshot from 2025-01-14 12-06-34
The application UI displays incorrectly. Works fine when I use AppImage directly.

@sarahec
Copy link
Contributor Author

sarahec commented Jan 14, 2025

@bet4it Please file a new bug and fill in the details there.

Also, are you on a ARM linux box? If not, the issue isn't coming from this PR and we'll need to work on it separately.

@sarahec sarahec requested a review from GaetanLepage January 14, 2025 17:42
@sarahec sarahec added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025
@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 373536


aarch64-linux

✅ 1 package built:
  • code-cursor

@sarahec sarahec added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person 6.topic: darwin Running or building packages on Darwin labels Jan 14, 2025
@GaetanLepage GaetanLepage requested a review from drupol January 14, 2025 19:25
@sarahec sarahec added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Jan 14, 2025
@GaetanLepage GaetanLepage merged commit a5924c2 into NixOS:master Jan 14, 2025
27 of 30 checks passed
@sarahec sarahec deleted the push-zmxovnqnpvsn branch January 15, 2025 17:06
@bet4it
Copy link

bet4it commented Jan 16, 2025

@sarahec I find this situation same on x86_64 and on previous version of code-cursor, so it's not related with this PR 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants