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

Will now do things pure Go instead of running commands #115

Merged
merged 24 commits into from
Dec 27, 2020
Merged

Will now do things pure Go instead of running commands #115

merged 24 commits into from
Dec 27, 2020

Conversation

CalebQ42
Copy link
Contributor

@CalebQ42 CalebQ42 commented Dec 7, 2020

If it's working

I tried to add the ability to use my squashfs library while still falling back to using commands if it fails.

@CalebQ42
Copy link
Contributor Author

CalebQ42 commented Dec 7, 2020

Part 1 of solving #1

@probonopd
Copy link
Owner

"Excellent" is an understatement. This is outstanding! 🥇

Only ask: Could you integrate https://github.com/CalebQ42/GoAppImage here, so that we don't need that repository as an additional dependency?

Thanks a lot! 👍

@CalebQ42
Copy link
Contributor Author

CalebQ42 commented Dec 7, 2020

Yeah, I can definitely get working on that soon.

@probonopd probonopd marked this pull request as draft December 7, 2020 19:46
@CalebQ42
Copy link
Contributor Author

CalebQ42 commented Dec 8, 2020

Just for some clarification, did you want me to use CalebQ42/GoAppImage for the tools, or moving the library to this repo?

@probonopd
Copy link
Owner

I would like https://github.com/CalebQ42/GoAppImage to not be needed by making the necessary changes in this reporitory, if this makes sense to you.

@CalebQ42
Copy link
Contributor Author

CalebQ42 commented Dec 9, 2020

So thinking about it, it might be better if it remains it's own library mainly because

  • It's easier to import when it's on its own.
  • Including it in this repo would make versioning with go modules difficult, especially with appimaged and appimagetool in the same repo

Since AppImage is yours, I'll defer to what you want to do. I'd also be more then happy to transfer ownership to GoAppImage to keep all AppImage under your purview.

Try to prefil some desktop entry information from the appimage's desktop file
@probonopd
Copy link
Owner

Thanks a lot @CalebQ42. Just to be clear, I am tatlking "just" about https://github.com/CalebQ42/GoAppImage, not about your squashfs library. By looking at https://github.com/CalebQ42/GoAppImage, I wonder why the filesin that could not go into this repo? Am I missing something? Of course you would get full credit for all of your work, and we both are using the MIT License anyway. I am just trying to make things easier for users of AppImage-related Go code by having one, rather than a multitude of somehow intertwined, repositories that are partly duplicating code.

It's easier to import when it's on its own.

How is that, can you give an example? Could we make the necessary changes in this repository to make it easier to import?

Including it in this repo would make versioning with go modules difficult, especially with appimaged and appimagetool in the same repo

Why? Having everything related to AppImage in one repo would ensure the correct versions of everything are always in sync, and are maintained and tested together. Wouldn't that be better? For example, if a bug occurs in any of your code that would prevent the creation or execution of AppImages, it would make the builds in this repository fail, and hence add an additional level of testing.

With squashfs, I think this is a different topic (not specific to AppImage), so that justifies having its own repository.

Can you follow my reasoning? Am I missing something?

@CalebQ42
Copy link
Contributor Author

CalebQ42 commented Dec 9, 2020

Very good points. Honestly yesterday I was feeling a bit under the weather and ended up just staying in bed all day and just thought about it a bit much I think. When talking about it being easier to import, I was more talking about the discovery of the library since it might be a bit confusing it being bundled in with the tools, but that's very minor.

But yeah, I'll definitely work on integrating CalebQ42/GoAppImage here.

@CalebQ42
Copy link
Contributor Author

I just roughly added the library. I think before I take the time to actually add it to the tools, I'll probably get the library to a somewhat stable state and remove things that only make sense for appimaged.

Removed code that's only valuable to appimaged
Used goto statements to help with command fallback.
@probonopd
Copy link
Owner

What happened to LaunchMostRecentAppImage?

@CalebQ42
Copy link
Contributor Author

It should still be there for appimaged, I've just removed it from the library because it assumes that there are AppImages integrated into the system (which the library does not). I'm making the library in a more general fashion that any program can use. More specifically, I'm largely thinking about LinuxPA and what sort of information would be useful in that sort of context.

When I get around to integrating it into appimaged, I'll probably end up just extending the struct with the information that's useful in that context, such as it's md5 value, where the thumbnail and desktop files should be store, etc.

Will now try to parse the desktop file at creation and get the AppImage's name from it
ExtractFile can now (for type 2 with a working reader) resolve symlinks
Added the ability to get the icon (a bit half baked right now though)
Changed getFSTime to ModTime to be consistent with os.File.
@probonopd
Copy link
Owner

This will be awesome, native type-1 and type-2-squashfs in Golang... 👍

@CalebQ42
Copy link
Contributor Author

So, after a couple days of being way too busy and looking at basically all iso9660 libraries, it looks like none of them support the Rock Ridge extension. This means no uppercase characters in filenames, and, more importantly, no symlink support.

Since this would force me to fallback to commands if either of these happen, and type-1 appimages SHOULD be fairly rare (I personally had to dig into your old bintray repo to find one), I'll probably have type-1 appimages just use commands, at least for now.

Implemented symlink resolution for type 1 AppImages.
Still need to make it work with the main library though.
archiveReader should work properly for both type1 and type2
Mainly just need to test everything right now.
@CalebQ42
Copy link
Contributor Author

Okay, I THINK I'm finally done with the goappimage library. I still need to run some tests to make sure it's all working as intended (and I need to sleep right now), but I think I can finally start integrating it into appimaged.

As something that was originally just me going to integrate native squashfs, this took A LOT longer then intended, lol.

@CalebQ42
Copy link
Contributor Author

@probonopd I've figured out why TravisCI is failing right now. It seems a recent change to otiai10/copy doesn't work on GOARCH=386. Even when setting the go.mod to an earlier version, it still downloads and tries to build the library, which is causing the issue. I submitted a PR to fix the issue.

Reverted some changes I made to debug TravisCI failing
@probonopd
Copy link
Owner

It seems a recent change to otiai10/copy doesn't work on GOARCH=386.

Thanks for finding this out @CalebQ42. It's really hard to believe that something as "simple" as copying a directory and its contents isn't part of Golang itself...

In any case, this also seems to be causing the build issues in
#110

@CalebQ42
Copy link
Contributor Author

It is a bit weird, but I think it's basically the same reason why C and C++ don't. At a low level each OS handles files very differently and making sure it's a 1:1 copy is a bit difficult. That being said, they really should make it a part of the standard libraries.

I'm working on some stuff I'm going to upload in a bit, and I'll probably switch it to using my fork and see if it breaks on other GOARCH's.

Changed to CalebQ42/copy temporarily
All file handling is done through archivereader so things can be much cleaner.
Cleaned up thumbnail getting
Changed back to otiai10/copy
@CalebQ42
Copy link
Contributor Author

I think I'm FINALLY done (for now). I'm pretty sure goappimage has nearly all the abilities that are necessary and useful. I've integrated it into appimaged. The main benefits is we now start with the AppImage's desktop file and symlink's are handled by goappimage so we don't have to worry about that anymore.

Admittedly, I haven't gotten around to building and testing appimaged to debug it, but I haven't made any large changes to it, just how it gets files.

@CalebQ42 CalebQ42 marked this pull request as ready for review December 26, 2020 12:37
@probonopd
Copy link
Owner

Thank you @CalebQ42. Very, very useful PR. Now we need appimaged builds to test ;-)
Do you think this is ready for being merged? I'd be quick to revert if something breaks.

@CalebQ42
Copy link
Contributor Author

Okay, so I got around to testing appimaged and it's basically all working. The ONLY problem I've had is that the balenaEtcher AppImage's desktop file is missing it's first byte when reading it for some reason. This might be an issue with my squashfs library, though I'm not having this issue with the Subsurface AppImage. I'll have to check this out real quick.

@probonopd
Copy link
Owner

Thanks for the heads-up @CalebQ42.

Fixed some issues with thumbnails and desktop files
@CalebQ42
Copy link
Contributor Author

@probonopd It should now be ready for merging. There was an issue with my squashfs library, and I found a couple of issues with thumbnails and desktop files and so they should work properly now.

@probonopd probonopd merged commit fb78712 into probonopd:master Dec 27, 2020
@probonopd
Copy link
Owner

Fingers crossed!

@probonopd
Copy link
Owner

Unfortunately the builds now fail:
https://travis-ci.com/github/probonopd/go-appimage

Any idea @CalebQ42?

@CalebQ42
Copy link
Contributor Author

@probonopd Try rebuilding. It's due to one of the dependencies that is a bit weird how they do major versions that can randomly fail. If I remember right, it's more predictable on newer version of go. I might to a PR to update the version of go used for building, if you want.

Incidentally, my fork (which is up to date) is building just fine: https://travis-ci.com/github/CalebQ42/go-appimage

@probonopd
Copy link
Owner

Thanks @CalebQ42, triggered a rebuild.

@probonopd
Copy link
Owner

Nope. Getting

package github.com/probonopd/go-appimage/src/appimaged
	imports github.com/pierrec/lz4/v4: cannot find package "github.com/pierrec/lz4/v4" in any of:
	/usr/local/go/src/github.com/pierrec/lz4/v4 (from $GOROOT)
	/tmp/go/src/github.com/pierrec/lz4/v4 (from $GOPATH)

@CalebQ42
Copy link
Contributor Author

@probonopd I'm definitely starting to remembering the issues I've had with this library before. Basically, if you get it downloaded and working, then you never have issues again. I did delete my GOPATH and was able to get the error locally, but fix it by running go get on a git clone'd copy of the repository. I'll submit a PR with a modified build script in just a minute.

This seems to be just some weird quirk of go modules and this particular library.

@probonopd
Copy link
Owner

Hello @CalebQ42 can we remove mksquashfs and unsquashfs from our AppImages?

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