Make AppImageTool OSX friendlier #466

teras · 2017-09-02T20:58:39Z

No description provided.

probonopd · 2017-09-03T00:09:25Z

Please remove LGPL osx_elf.h as its license is incompatible with this project. A few definitions from it (the ones regarding Intel and ARM 32-bit and 64-bit) should be sufficient, after all.

TheAssassin · 2017-09-03T00:15:17Z

The entire squashfs_osx.patch file is indented with tabs instead of the 4 spaces used in the rest of the projects. Please fix that.

I'll post a more detailed review when I get to it.

teras · 2017-09-03T01:45:23Z

About osx_elf : done. I am not sure if this changes anything license wise though.
It doesn't mean anything else if you are linking with system file or provided file; the license is the same.

About tabs, I hate tabs too, but this is how the original source and patch is. I'd prefer not to change anything on a code that is outside the scope of this codebase.

teras · 2017-09-03T01:47:47Z

I created also a patch to support gpg & shashum, which are almost the same with their equivalent gpg2 and sha256sum. My only question is how to display the name to the user. I preferred the gpg[2] and sha[256]sum, which is more condensed. Ideas welcome.

TheAssassin

I see a lot of potential for improvements in this PR. You tried to follow the overall code style at least in the files which were existing already, but the new file breaks with the existing style.

At some point I stopped analyzing the PR, as the review got quite long. Many of the mentioned points are valid for other sections as well, e.g., my words about the architecture code.

I'd ask you to fix some of the issues, or to start a discussion. After fixing these points, I'd review the PR again.

TheAssassin · 2017-09-03T20:11:01Z

appimagetool.c

+#define fARCH_i386   0
+#define fARCH_x86_64 1
+#define fARCH_arm   2
+#define fARCH_aarch64 3


This should be an enum. Even if it stayed a set of defines, the alignment of numbers should be fixed.

TheAssassin · 2017-09-03T20:11:56Z

appimagetool.c

-
-    if ((sp = strstr(line, search))) {
-        replacestr(line, search, replace);
+char* readFile(char* filename, int* size) {


As said before, I don't like this function's signature. It should be changed to void. Or, while I think about it, a bool would be a lot better, that returns false in case of errors.

TheAssassin · 2017-09-03T20:14:52Z

appimagetool.c

-                // sprintf (guessed_bin_path, "%s/usr/bin/%s", source,  g_strsplit_set(get_desktop_entry(kf, "Exec"), " ", -1)[0]);
-                // archfile = guessed_bin_path;
-                archfile = "/proc/self/exe";
+            char archs[4];


Why is this a char array?

TheAssassin · 2017-09-03T20:25:12Z

appimagetool.c

-            {
-                printf("The architecture could not be determined, assuming 'all'\n");
-                arch="all";
+            if (archs[fARCH_x86_64]) {


I'm not very fond of this mix of representation and actual data gathering. I'd rather have used a little for loop to count the architectures, and then render the output. Also, you're overwriting the arch variable even when there's multiple architectures. I'd rather have rendered the actually found architectures into the error message, and then created the arch variable afterwards.

TheAssassin · 2017-09-03T20:32:57Z

appimagetool.c

+        char* data;
+        if (runtime_file) {
+            data = readFile(runtime_file, &size);
+            if (!data)


As said above, I don't like the API of this function, and therefore don't like this kind of error handling. Comparing a pointer here can be really problematic, and is not really straightforward. If any, you need to do both, check for the pointer and check for a return value from a boolean function.

Added a null check, as you asked

TheAssassin · 2017-09-03T20:52:30Z

getsection.c

@@ -1,4 +1,9 @@
-#include <elf.h>
+#ifdef __APPLE__
+    #include "osx_elf.h"


See comment for elf.c.

Again, I didn't understand that

TheAssassin · 2017-09-03T20:53:44Z

osx_elf.h

@@ -0,0 +1,103 @@
+/* This is a derivative work of the elf.h file from the GNU C Library */


This line suggests that the file is licensed under the LGPL. This license is incompatible with the rest of the project. This is a major problem, and thus the PR cannot be merged.

Correct, I'll copy the symbols from https://github.com/torvalds/linux/blob/master/include/uapi/linux/elf.h

is that better?

TheAssassin · 2017-09-03T20:53:56Z

osx_elf.h

+/* This is a derivative work of the elf.h file from the GNU C Library */
+
+#ifndef _ELF_H
+#define	_ELF_H 1


This tab needs to be removed.

it were on the original file - now the whole file is properly indented.

TheAssassin · 2017-09-03T20:54:35Z

osx_elf.h

+
+typedef struct
+{
+  unsigned char	e_ident[EI_NIDENT];	/* Magic number and other info */


Tabs for indentation are not used by the rest of the project. You will have to change the indentation of the entire file.

TheAssassin · 2017-09-03T20:55:48Z

squashfs_osx.patch

@@ -0,0 +1,434 @@
+diff --git a/squashfs-tools/action.c b/squashfs-tools/action.c


You are right, the tabs in this file are totally justified by the fact that the upstream project uses them.

teras · 2017-09-04T15:17:21Z

The changes have been applied... please have a look

TheAssassin · 2017-09-04T18:30:04Z

light_elf.h

+
+__END_DECLS
+
+#endif /* elf.h */


This file is still licensed as GNU GPL. This license is not compatible with the rest of the code base. This is a really big problem, and we cannot risk a GPL violation by relicensing the extracted code. You will have to write it yourself, I guess, and change the variable and macro names etc. The file doesn't have to be compatible to any other code any more, especially since this is going to be used on both Linux and OS X.

teras · 2017-09-08T16:36:54Z

I have the feeling that #471 is mutually exclusive with this, patch, since it references the exact same location with memory management, while this patch also takes into account external binary files. My humble opinion is, if possible, to fix possible issues with this patch, apply it if you want and then take care of #471, who is indeed dependent on this tool being multi-platform first. Otherwise external files patch will not apply and maybe it will fail on other locations too.

EDIT: I just saw that you have already applied the patches I was afraid of, thus this patch does not apply clean any more...

probonopd · 2017-09-08T16:43:05Z

#471 is not applied yet, since it doesn't build "green" yet...

probonopd · 2017-09-11T20:10:55Z

Thank you, great work @teras and throrough reviews @TheAssassin. It builds "green" now, and I'd really like to get this merged soon. But that will require all GPL code to be replaced by something that we can license under the MIT license. This is important for adoption of AppImage by some of our business users (yes, Microsoft and IBM are already using AppImage, too, and we don't want to scare them away.)

teras · 2017-09-18T10:36:04Z

@probonopd we had a discussion about that with @TheAssassin and we concluded that what the last thing I did was the best thing to do. Please if possible discuss with him about this.

TheAssassin · 2017-09-18T13:11:39Z

@teras, @probonopd, I should get to merging everything manually soon. I'll fix the remaining issues, merge #470 and #471, and then everything should work fine.

@probonopd we will now have to provide runtimes for embedding built by Travis I guess. I'll implement that, too.

probonopd · 2017-09-18T18:15:50Z

Thank you @teras @TheAssassin

TheAssassin · 2017-09-26T18:06:33Z

Sorry for the delay. I'm going to review and most likely merge this tonight.

TheAssassin · 2017-09-26T18:56:16Z

Not sure what's going on in the x86_64 build. The 32-bit one is broken due to a dependency issue. I'm going to fix the latter first.

TheAssassin · 2017-09-26T19:58:51Z

As mentioned in #433, the builds are failing due to issues with SourceForge. We'll have to wait for this to get fixed. It also shows how fragile our build system is. We should mirror those tarballs.

probonopd · 2017-09-29T17:39:18Z

Looks like the checks are passing now. Ready to merge @TheAssassin?

TheAssassin · 2017-09-30T11:17:37Z

I have been working on the legal implications of the GPL code included in this repository. I have come up with the following explanation.

The file is and will remain GPLv2. The copyright notice must stay, and should not be modified (although the file may be modified, but when it is modified will also remain GPLv2). Especially the year and author name are important information.

The file only contains definitions of data types and other values. According to Richard Stallman [1], when using this file to create a program, it will not create a derivative work and therefore, appimagetool is not subject to the GPLv2. This is important to know, as this is what @probonopd warned about. I wanted to revise and explicitly state the situation again. (I know that @teras and I knew this already, based on [2].) So, shipping this GPLv2 file along with the MIT licensed code is okay.

I will have to verify whether @teras made changes to the file, as then, we should update the copyright notice in the header file before merging.

Sorry for the additional delay.

[1] http://lkml.iu.edu/hypermail/linux/kernel/0301.1/0362.html
[2] https://elinux.org/Legal_Issues#Use_of_kernel_header_files_in_user-space

TheAssassin · 2017-09-30T11:40:47Z

@teras did make changes, and stated this implicitly. I've fixed this. As @probonopd forces Travis to run successfully before being able to merge, we'll have to wait before finally merging this PR (although this is not really necessary).

TheAssassin · 2017-09-30T11:41:51Z

Thank you very much @teras for your great work! I hope users will be able to make use of it.

teras · 2017-09-30T21:20:40Z

Hello all and thanks for accepting the patch! The only thing I did with elf include file was to remove all not-required content (as suggested) and keep only the dependencies. BTW, if the embedded method catches on, this method could be eliminated altogether I think.

…

On Sat, Sep 30, 2017 at 2:41 PM, TheAssassin ***@***.***> wrote: Thank you very much @teras <https://github.com/teras> for your great work! I hope users will be able to make use of it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#466 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkKuGcLK_DbOfUVj2U6Kq3eoCIidZk4ks5snikCgaJpZM4PLAm9> .

-- Panayotis Katsaloulis

develar · 2017-10-01T07:54:57Z

@teras To build appimagetool (./build-tool-osx.sh) on macOS, you need to

brew install libtool glib argp-standalone
brew install Caskroom/cask/osxfuse

right?

develar · 2017-10-01T08:04:53Z

To be able to use appimagetool on macOS, I need runtime, but no runtime files in the https://github.com/AppImage/AppImageKit/releases/tag/continuous (I can build it on Linux, of course, but it will be better if it will be provided to ensure that it is compiled on correct OS and so on).

TheAssassin · 2017-10-01T09:05:47Z

You are absolutely right, and I found so elsewhere, too. Would you like to create an issue or even a PR to build runtimes for download on Travis?

teras · 2017-10-01T10:26:18Z

@develar

Yes, I had to install a couple of things from vanilla brew, no issues or special treatment there. I can remove brew from my system and see what's missing. Of course, it would make more sense (and in accordance with the current procedure) to have a test environment that would download the packages on the fly and try them. This will save time, if a brew dependency package changes and breaks some things (instead of checking the procedure once - now).

As a parallel feature, why not consider creating a brew especially for appimagekit?

TheAssassin · 2017-10-01T12:19:53Z

You can always do this, but I guess the AppImage team wouldn't officially support it. Also, I'd suggest to discuss this in a separate issue.

teras added 2 commits September 2, 2017 23:30

Make code OSX friendlier

a71f7db

fix zero typo

4242346

probonopd requested a review from TheAssassin September 2, 2017 21:11

fail if no arch parameter is provided

43e5878

probonopd mentioned this pull request Sep 2, 2017

Linux auto-update electron-userland/electron-builder#1138

Closed

Initial version of osx build script

dfa45a6

teras force-pushed the appimagetool/master branch from 653a8e3 to dfa45a6 Compare September 2, 2017 23:29

teras added 2 commits September 3, 2017 02:55

Update patch

e27a72b

make mksquashfs build enabled again ; clean up a bit

3cce28b

teras added 3 commits September 3, 2017 04:22

thinner osx_elf.h

6fc20b9

do not need to repatch

4d2468d

support gpg & shasum under OSX

8244870

teras added 5 commits September 3, 2017 15:20

support multi-arch and executables alongside so files

db87b5a

Merge branch 'appimagetool/master' into appimagetool/master

96edc62

be a bit more consistent with debugging

199633e

make struct safer

46672f1

renamed ARCHs just in case

6df5fc0

teras force-pushed the appimagetool/master branch from 1b9f560 to 6df5fc0 Compare September 3, 2017 13:32

teras added 4 commits September 3, 2017 16:43

remove typedef

643edfc

Using arrays instead of struct

30fbaab

strict null parameter check & support arm archs

777dfb5

check for i486 i586 i686

2416bce

teras force-pushed the appimagetool/master branch from 2698d10 to 2416bce Compare September 3, 2017 15:09

remove debug information for file

2619d67

TheAssassin requested changes Sep 3, 2017

View reviewed changes

teras force-pushed the appimagetool/master branch from f68f1f5 to 4b0cb26 Compare September 4, 2017 06:46

TheAssassin reviewed Sep 4, 2017

View reviewed changes

teras force-pushed the appimagetool/master branch from 0eee828 to 8db00f0 Compare September 4, 2017 18:50

2nd iteration - hopefully last

c7fe818

teras force-pushed the appimagetool/master branch from 8db00f0 to c7fe818 Compare September 4, 2017 18:51

TheAssassin mentioned this pull request Sep 7, 2017

Change embedding of runtime to platform agnostic way #470

Closed

Merge branch 'appimagetool/master' into appimagetool/master

b114be9

Merge branch 'appimagetool/master' into appimagetool/master

2a7510f

Update light_elf.h

856dfe4

TheAssassin approved these changes Sep 30, 2017

View reviewed changes

TheAssassin merged commit cc63970 into AppImage:appimagetool/master Sep 30, 2017

		@@ -0,0 +1,103 @@
		/* This is a derivative work of the elf.h file from the GNU C Library */

		@@ -0,0 +1,434 @@
		diff --git a/squashfs-tools/action.c b/squashfs-tools/action.c


		__END_DECLS

		#endif /* elf.h */

Make AppImageTool OSX friendlier #466

Make AppImageTool OSX friendlier #466

Conversation

teras commented Sep 2, 2017

probonopd commented Sep 3, 2017

TheAssassin commented Sep 3, 2017

teras commented Sep 3, 2017

teras commented Sep 3, 2017

TheAssassin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teras commented Sep 4, 2017

Choose a reason for hiding this comment

teras commented Sep 8, 2017 • edited Loading

probonopd commented Sep 8, 2017

probonopd commented Sep 11, 2017 • edited Loading

teras commented Sep 18, 2017

TheAssassin commented Sep 18, 2017

probonopd commented Sep 18, 2017

TheAssassin commented Sep 26, 2017

TheAssassin commented Sep 26, 2017

TheAssassin commented Sep 26, 2017

probonopd commented Sep 29, 2017

TheAssassin commented Sep 30, 2017 • edited Loading

TheAssassin commented Sep 30, 2017

TheAssassin commented Sep 30, 2017

teras commented Sep 30, 2017 via email

develar commented Oct 1, 2017 • edited Loading

develar commented Oct 1, 2017

TheAssassin commented Oct 1, 2017

teras commented Oct 1, 2017 • edited Loading

TheAssassin commented Oct 1, 2017

teras commented Sep 8, 2017 •

edited

Loading

probonopd commented Sep 11, 2017 •

edited

Loading

TheAssassin commented Sep 30, 2017 •

edited

Loading

develar commented Oct 1, 2017 •

edited

Loading

teras commented Oct 1, 2017 •

edited

Loading