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.

Sign up for GitHub

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

Make AppImageTool OSX friendlier #466

Merged
merged 24 commits into from
Sep 30, 2017
Merged

Make AppImageTool OSX friendlier #466

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a71f7db
Make code OSX friendlier
teras Sep 2, 2017
4242346
fix zero typo
teras Sep 2, 2017
43e5878
fail if no arch parameter is provided
teras Sep 2, 2017
dfa45a6
Initial version of osx build script
teras Sep 2, 2017
e27a72b
Update patch
teras Sep 2, 2017
3cce28b
make mksquashfs build enabled again ; clean up a bit
teras Sep 3, 2017
6fc20b9
thinner osx_elf.h
teras Sep 3, 2017
4d2468d
do not need to repatch
teras Sep 3, 2017
8244870
support gpg & shasum under OSX
teras Sep 3, 2017
db87b5a
support multi-arch and executables alongside so files
teras Sep 3, 2017
96edc62
Merge branch 'appimagetool/master' into appimagetool/master
teras Sep 3, 2017
199633e
be a bit more consistent with debugging
teras Sep 3, 2017
46672f1
make struct safer
teras Sep 3, 2017
6df5fc0
renamed ARCHs just in case
teras Sep 3, 2017
643edfc
remove typedef
teras Sep 3, 2017
30fbaab
Using arrays instead of struct
teras Sep 3, 2017
777dfb5
strict null parameter check & support arm archs
teras Sep 3, 2017
2416bce
check for i486 i586 i686
teras Sep 3, 2017
2619d67
remove debug information for file
teras Sep 3, 2017
5a9ebc9
changes as requested by upstream
teras Sep 4, 2017
c7fe818
2nd iteration - hopefully last
teras Sep 4, 2017
b114be9
Merge branch 'appimagetool/master' into appimagetool/master
teras Sep 8, 2017
2a7510f
Merge branch 'appimagetool/master' into appimagetool/master
teras Sep 18, 2017
856dfe4
Update light_elf.h
TheAssassin Sep 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
249 changes: 171 additions & 78 deletions appimagetool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,21 @@

#include <unistd.h>
#include <string.h>
#include <limits.h>

#include "elf.h"
#include "getsection.h"

#ifdef __linux__
#define HAVE_BINARY_RUNTIME
extern int _binary_runtime_start;
extern int _binary_runtime_end;
#endif

#define fARCH_i386 0
#define fARCH_x86_64 1
#define fARCH_arm 2
#define fARCH_aarch64 3
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.


static gchar const APPIMAGEIGNORE[] = ".appimageignore";
static char _exclude_file_desc[256];
Expand All @@ -68,6 +77,7 @@ gchar *bintray_user = NULL;
gchar *bintray_repo = NULL;
gchar *sqfs_comp = "gzip";
gchar *exclude_file = NULL;
gchar *runtime_file = NULL;

// #####################################################################

Expand Down Expand Up @@ -221,31 +231,93 @@ int validate_desktop_file(char *file) {
* }
*/

gchar* find_first_matching_file(const gchar *real_path, const gchar *pattern) {
/* in-place modification of the string, and assuming the buffer pointed to by
* line is large enough to hold the resulting string*/
static void replacestr(char *line, const char *search, const char *replace)
{
char *sp = NULL;

if ((sp = strstr(line, search)) == NULL) {
return;
}
int search_len = strlen(search);
int replace_len = strlen(replace);
int tail_len = strlen(sp+search_len);

memmove(sp+replace_len,sp+search_len,tail_len+1);
memcpy(sp, replace, replace_len);

/* Do it recursively again until no more work to do */

if ((sp = strstr(line, search))) {
replacestr(line, search, replace);
}
}

void guess_arch(const gchar *archfile, char* archs) {
gchar *carch;
char line[PATH_MAX];
char command[PATH_MAX];
sprintf(command, "/usr/bin/file -L -N -b %s", archfile);
FILE* fp = popen(command, "r");
if (fp == NULL)
die("Failed to run file command");
fgets(line, sizeof (line) - 1, fp);
pclose(fp);
carch = g_strsplit_set(line, ",", -1)[1];
if (carch) {
carch = g_strstrip(carch);
if (carch) {
replacestr(carch, "-", "_");
replacestr(carch, " ", "_");
if (g_ascii_strncasecmp("i386", carch, 20) == 0
|| g_ascii_strncasecmp("i486", carch, 20) == 0
|| g_ascii_strncasecmp("i586", carch, 20) == 0
|| g_ascii_strncasecmp("i686", carch, 20) == 0
|| g_ascii_strncasecmp("intel_80386", carch, 20) == 0
|| g_ascii_strncasecmp("intel_80486", carch, 20) == 0
|| g_ascii_strncasecmp("intel_80586", carch, 20) == 0
|| g_ascii_strncasecmp("intel_80686", carch, 20) == 0
) {
archs[fARCH_i386] = 1;
if (verbose)
fprintf(stderr, "File used for determining architecture i386: %s\n", archfile);
} else if (g_ascii_strncasecmp("x86_64", carch, 20) == 0) {
archs[fARCH_x86_64] = 1;
if (verbose)
fprintf(stderr, "File used for determining architecture x86_64: %s\n", archfile);
} else if (g_ascii_strncasecmp("arm", carch, 20) == 0) {
archs[fARCH_arm] = 1;
if (verbose)
fprintf(stderr, "File used for determining architecture ARM: %s\n", archfile);
} else if (g_ascii_strncasecmp("arm_aarch64", carch, 20) == 0) {
archs[fARCH_aarch64] = 1;
if (verbose)
fprintf(stderr, "File used for determining architecture ARM aarch64: %s\n", archfile);
}
}
}
}

void find_arch(const gchar *real_path, const gchar *pattern, char* archs) {
GDir *dir;
gchar *full_name;
gchar *resulting;
dir = g_dir_open(real_path, 0, NULL);
if (dir != NULL) {
const gchar *entry;
while ((entry = g_dir_read_name(dir)) != NULL) {
full_name = g_build_filename(real_path, entry, NULL);
if (! g_file_test(full_name, G_FILE_TEST_IS_DIR)) {
if(g_pattern_match_simple(pattern, entry))
return(full_name);
}
else {
resulting = find_first_matching_file(full_name, pattern);
if(resulting)
return(resulting);
if (g_file_test(full_name, G_FILE_TEST_IS_DIR)) {
find_arch(full_name, pattern, archs);
} else if (g_file_test(full_name, G_FILE_TEST_IS_EXECUTABLE) || g_pattern_match_simple(pattern, entry) ) {
guess_arch(full_name, archs);
}
}
g_dir_close(dir);
}
else {
g_warning("%s: %s", real_path, g_strerror(errno));
}
return NULL;
}

gchar* find_first_matching_file_nonrecursive(const gchar *real_path, const gchar *pattern) {
Expand Down Expand Up @@ -277,27 +349,22 @@ gchar* get_desktop_entry(GKeyFile *kf, char *key) {
return value;
}

/* in-place modification of the string, and assuming the buffer pointed to by
* line is large enough to hold the resulting string*/
static void replacestr(char *line, const char *search, const char *replace)
{
char *sp = NULL;

if ((sp = strstr(line, search)) == NULL) {
return;
}
int search_len = strlen(search);
int replace_len = strlen(replace);
int tail_len = strlen(sp+search_len);

memmove(sp+replace_len,sp+search_len,tail_len+1);
memcpy(sp, replace, replace_len);

/* Do it recursively again until no more work to do */

if ((sp = strstr(line, search))) {
replacestr(line, search, replace);
char* readFile(char* filename, int* size) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

FILE* f = fopen(filename, "rb");
if (f==NULL) {
*size = 0;
return 0;
}

fseek(f, 0, SEEK_END);
long fsize = ftell(f);
fseek(f, 0, SEEK_SET);

char *buffer = malloc(fsize);
fread(buffer, fsize, 1, f);
fclose(f);
*size = (int)fsize;
return buffer;
}

// #####################################################################
Expand All @@ -310,10 +377,11 @@ static GOptionEntry entries[] =
{ "bintray-repo", 0, 0, G_OPTION_ARG_STRING, &bintray_repo, "Bintray repository", NULL },
{ "version", 0, 0, G_OPTION_ARG_NONE, &version, "Show version number", NULL },
{ "verbose", 'v', 0, G_OPTION_ARG_NONE, &verbose, "Produce verbose output", NULL },
{ "sign", 's', 0, G_OPTION_ARG_NONE, &sign, "Sign with gpg2", NULL },
{ "sign", 's', 0, G_OPTION_ARG_NONE, &sign, "Sign with gpg[2]", NULL },
{ "comp", 0, 0, G_OPTION_ARG_STRING, &sqfs_comp, "Squashfs compression", NULL },
{ "no-appstream", 'n', 0, G_OPTION_ARG_NONE, &no_appstream, "Do not check AppStream metadata", NULL },
{ "exclude-file", 0, 0, G_OPTION_ARG_STRING, &exclude_file, _exclude_file_desc, NULL },
{ "runtime-file", 0, 0, G_OPTION_ARG_STRING, &runtime_file, "Runtime file to use", NULL },
{ G_OPTION_REMAINING, 0, 0, G_OPTION_ARG_FILENAME_ARRAY, &remaining_args, NULL, NULL },
{ 0,0,0,0,0,0,0 }
};
Expand Down Expand Up @@ -373,10 +441,10 @@ main (int argc, char *argv[])
if(! no_appstream)
if(! g_find_program_in_path ("appstreamcli"))
g_print("WARNING: appstreamcli is missing, please install it if you want to use AppStream metadata\n");
if(! g_find_program_in_path ("gpg2"))
g_print("WARNING: gpg2 is missing, please install it if you want to create digital signatures\n");
if(! g_find_program_in_path ("sha256sum"))
g_print("WARNING: sha256sum is missing, please install it if you want to create digital signatures\n");
if(! g_find_program_in_path ("gpg2") && ! g_find_program_in_path ("gpg"))
g_print("WARNING: gpg[2] is missing, please install it if you want to create digital signatures\n");
if(! g_find_program_in_path ("sha256sum") && ! g_find_program_in_path ("shasum"))
g_print("WARNING: sha[256]sum is missing, please install it if you want to create digital signatures\n");

if(!&remaining_args[0])
die("SOURCE is missing");
Expand Down Expand Up @@ -428,38 +496,37 @@ main (int argc, char *argv[])
FILE *fp;
/* If no $ARCH variable is set check a file */
if (!arch) {
gchar *archfile = NULL;
/* We use the next best .so that we can find to determine the architecture */
archfile = find_first_matching_file(source, "*.so.*");
if(!archfile)
{
/* If we found no .so we try to guess the main executable - this might be a script though */
// char guessed_bin_path[PATH_MAX];
// 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];
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a char array?

find_arch(source, "*.so.*", archs);
int countArchs = 0;
if (archs[fARCH_i386]) {
arch = "i386";
countArchs++;
}
if(verbose)
fprintf (stderr,"File used for determining architecture: %s\n", archfile);

char line[PATH_MAX];
char command[PATH_MAX];
sprintf (command, "/usr/bin/file -L -N -b %s", archfile);
fp = popen(command, "r");
if (fp == NULL)
die("Failed to run file command");
fgets(line, sizeof(line)-1, fp);
arch = g_strstrip(g_strsplit_set(line, ",", -1)[1]);
replacestr(arch, "-", "_");
fprintf (stderr,"Arch: %s\n", arch+1);
pclose(fp);

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

arch = "x86_64";
countArchs++;
}
if (archs[fARCH_arm]) {
arch = "ARM";
countArchs++;
}
if (archs[fARCH_aarch64]) {
arch = "ARM aarch64";
countArchs++;
}
if (countArchs!=1) {
if (countArchs<1)
fprintf(stderr, "Unable to guess the architecture of the AppDir source directory \"%s\"\n", remaining_args[0]);
else
fprintf(stderr, "More than one architectures were found of the AppDir source directory \"%s\"\n", remaining_args[0]);
fprintf(stderr, "A valid architecture with the ARCH environmental variable should be provided\ne.g. ARCH=x86_64 %s", argv[0]),
die(" ...");
}
}
if (verbose)
fprintf(stderr, "Using architecture %s\n", arch);

char app_name_for_filename[PATH_MAX];
sprintf(app_name_for_filename, "%s", get_desktop_entry(kf, "Name"));
Expand Down Expand Up @@ -570,10 +637,22 @@ main (int argc, char *argv[])
* should hopefully change that. */

fprintf (stderr, "Generating squashfs...\n");
/* runtime is embedded into this executable
* http://stupefydeveloper.blogspot.de/2008/08/cc-embed-binary-data-into-elf.html */
int size = (int)((void *)&_binary_runtime_end - (void *)&_binary_runtime_start);
char *data = (char *)&_binary_runtime_start;
int size = 0;
char* data;
if (runtime_file) {
data = readFile(runtime_file, &size);
if (!data)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a null check, as you asked

die("Unable to load provided runtime file");
} else {
#ifdef HAVE_BINARY_RUNTIME
/* runtime is embedded into this executable
* http://stupefydeveloper.blogspot.de/2008/08/cc-embed-binary-data-into-elf.html */
size = (int)((void *)&_binary_runtime_end - (void *)&_binary_runtime_start);
data = (char *)&_binary_runtime_start;
#else
die("No runtime file was provided");
Copy link
Member

Choose a reason for hiding this comment

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

This kind of preprocessor usage is not too straightforward to me. Mixing the preprocessor and the C code if clause is not straightforward, and leads to a indistinct program flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I can't do that, since _binary_runtime_end and _binary_runtime_start does not exist in OSX. Thus this code needs to be into preprocessor.

Copy link
Member

Choose a reason for hiding this comment

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

I am pretty sure there is a better way to organize this code. The flow you propose isn't final, you could adapt it a bit. But this is not mandatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only thing that could be done is to put it in a separate method, so that it would be isolated - the logic there was already flawed anyway.

#endif
}
if (verbose)
printf("Size of the embedded runtime: %d bytes\n", size);

Expand All @@ -590,6 +669,8 @@ main (int argc, char *argv[])
fseek(fpdst, 0, SEEK_SET);
fwrite(data, size, 1, fpdst);
fclose(fpdst);
if(runtime_file)
free(data);
Copy link
Member

Choose a reason for hiding this comment

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

Rather check for the data pointer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK


fprintf (stderr, "Marking the AppImage as executable...\n");
if (chmod (destination, 0755) < 0) {
Expand Down Expand Up @@ -647,47 +728,59 @@ main (int argc, char *argv[])
if(sign){
/* The user has indicated that he wants to sign */
gchar *gpg2_path = g_find_program_in_path ("gpg2");
if (!gpg2_path)
gpg2_path = g_find_program_in_path ("gpg");
gchar *sha256sum_path = g_find_program_in_path ("sha256sum");
int req_shasum = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This could and should be a bool. Also, the variable name could be improved, it is not clear what it stands for.

if (!sha256sum_path) {
sha256sum_path = g_find_program_in_path ("shasum");
req_shasum = 1;
}
if(!gpg2_path){
fprintf (stderr, "gpg2 is not installed, cannot sign\n");
fprintf (stderr, "gpg[2] is not installed, cannot sign\n");
Copy link
Member

Choose a reason for hiding this comment

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

Throughout the file, you use this syntax to show that there's different programs on different platforms. Please make sure to inform the user which tools are used (gpg or gpg2, shasum or sha256sum) at some point. Then, these messages make a lot more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we discussed this yesterday, forgot to update. Done

}
else if(!sha256sum_path){
fprintf (stderr, "sha256sum is not installed, cannot sign\n");
fprintf (stderr, "sha[256]sum is not installed, cannot sign\n");
} else {
fprintf (stderr, "gpg2 and sha256sum are installed and user requested to sign, "
fprintf (stderr, "gpg[2] and sha[256]sum are installed and user requested to sign, "
"hence signing\n");
char *digestfile;
digestfile = br_strcat(destination, ".digest");
char *ascfile;
ascfile = br_strcat(destination, ".digest.asc");
if (g_file_test (digestfile, G_FILE_TEST_IS_REGULAR))
unlink(digestfile);
sprintf (command, "%s %s", sha256sum_path, destination);
if (req_shasum)
sprintf (command, "%s -a256 %s", sha256sum_path, destination);
else
sprintf (command, "%s %s", sha256sum_path, destination);
if(verbose)
fprintf (stderr, "%s\n", command);
fp = popen(command, "r");
if (fp == NULL)
die("sha256sum command did not succeed");
die("sha[256]sum command did not succeed");
char output[1024];
fgets(output, sizeof(output)-1, fp);
if(verbose)
printf("sha256sum: %s\n", g_strsplit_set(output, " ", -1)[0]);
printf("sha[256]sum: %s\n", g_strsplit_set(output, " ", -1)[0]);
FILE *fpx = fopen(digestfile, "w");
if (fpx != NULL)
{
fputs(g_strsplit_set(output, " ", -1)[0], fpx);
fclose(fpx);
}
if(WEXITSTATUS(pclose(fp)) != 0)
die("sha256sum command did not succeed");
int exit_status = pclose(fp);
Copy link
Member

Choose a reason for hiding this comment

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

Extraction to a variable doesn't make too much sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should under OSX unfortunately.

if(WEXITSTATUS(exit_status) != 0)
die("sha[256]sum command did not succeed");
if (g_file_test (ascfile, G_FILE_TEST_IS_REGULAR))
unlink(ascfile);
sprintf (command, "%s --detach-sign --armor %s", gpg2_path, digestfile);
if(verbose)
fprintf (stderr, "%s\n", command);
fp = popen(command, "r");
if(WEXITSTATUS(pclose(fp)) != 0) {
fprintf (stderr, "ERROR: gpg2 command did not succeed, could not sign, continuing\n");
exit_status = pclose(fp);
Copy link
Member

Choose a reason for hiding this comment

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

Overwriting the previously created variable doesn't make too much sense to me either.

If you'd want to do this, either use two separate variables, or create scopes (which isn't really ideal). But as said before, the compiler will optimize it out anyway, and the readability isn't improved significantly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK changed to two separate variables

if(WEXITSTATUS(exit_status) != 0) {
fprintf (stderr, "ERROR: gpg[2] command did not succeed, could not sign, continuing\n");
} else {
unsigned long sig_offset = 0;
unsigned long sig_length = 0;
Expand Down
Loading