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

Move from go get to git clone for fyne get #4713

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

andydotxyz
Copy link
Member

This clearly belongs on the new repo - but first I wanted to land it here to pick onto the release branch for quicker fix :)

Fixes #4684

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

See note inline about importing a deprecated package. Maybe there is something better we can use?

cmd/fyne/internal/commands/get.go Show resolved Hide resolved
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

This does not seem to clean up the temporary directory after installation. Trying to install twice (because I got an error the first time) results in this:

fatal: destination path '/tmp/v3' already exists and is not an empty directory.
exit status 128

I think you are missing a defer os.Remove() or similar.

I am not sure if this is due to this PR or not (my Go install did apparently not work at all with fyne get before this change) but I'm getting this error when trying to install Rymdport (fyne_demo works fine):

$ fyne get github.com/Jacalz/rymdport/v3

Cloning into '/tmp/v3'...
remote: Enumerating objects: 78, done.
remote: Counting objects: 100% (78/78), done.
remote: Compressing objects: 100% (75/75), done.
remote: Total 78 (delta 1), reused 39 (delta 1), pack-reused 0
Receiving objects: 100% (78/78), 170.37 KiB | 2.43 MiB/s, done.
Resolving deltas: 100% (1/1), done.

failed to set up installer: Missing application icon at "/tmp/v3/v3/Icon.png"

The error seems to be that the installer expects an Icon.png file and doesn't read FyneApp.toml? I'm quite sure that Rymdport used to work when installing through fyne get but the last time I tested was likely over a year ago (or more likely even longer).

cmd/fyne/internal/commands/get.go Outdated Show resolved Hide resolved
@andydotxyz
Copy link
Member Author

The error seems to be that the installer expects an Icon.png file and doesn't read
FyneApp.toml? I'm quite sure that Rymdport used to work when installing through fyne get
but the last time I tested was likely over a year ago (or more likely even longer)

This is not a regression, the icon handling is somewhat special for get but it should be resolved separately. You can pass "-icon" command to work around it for now.

@andydotxyz andydotxyz requested a review from Jacalz April 1, 2024 21:01
@andydotxyz andydotxyz added this to the v2.4.5 milestone Apr 2, 2024
@Jacalz Jacalz removed their request for review April 3, 2024 21:26
Copy link
Member

@Jacalz Jacalz left a comment

Choose a reason for hiding this comment

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

Hmm. This just seems to get stuck after cloning the repository from what I can tell. It resolves the deltas and then just seems to do absolutely nothing when trying to download fyne_demo. I'm not sure what is going on.

@Jacalz
Copy link
Member

Jacalz commented Apr 3, 2024

Actually: It might just be my Go install being stupidly slow at compiling for some reason. Will test again tomorrow.

@andydotxyz andydotxyz merged commit 7e354e0 into fyne-io:develop Apr 4, 2024
12 checks passed
@andydotxyz andydotxyz deleted the fix/fyneget122 branch April 8, 2024 09:00
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.

2 participants