Update README.md #4
Conversation
danbrakeley
left a comment
danbrakeley
left a comment
There was a problem hiding this comment.
So I like the intent of this PR, but I'm struggling with the practical problems of providing a complete set of build instructions for a CGO project like this. I'm very happy that all you needed to do was install Go, but I don't know that it will be that easy for everyone, as the primary dependency github.com/adrium/goheif is mostly C code, and requires a C toolchain to build.
I'm generally not a fan of CGO, but it was the only heif library I found at the time. CGO not only makes it harder to build, it may result in executables that depend on system-specific dynamic libraries. You can see in .github/workflows/release.yaml how I had to include specific dlls in order for the Windows exe to work for most users.
I think I'd be more open to a PR that modifies the release.yaml I linked above to add additional release executables for linux. I realize looking at release.yaml isn't the same as having step-by-step instructions, but it does provide a form of documentation that is actually exercised as code, and thus much more likely to be working.
| 3. Enter the repository and execute the [build.sh](https://github.com/danbrakeley/heic2png/blob/main/build.sh) script | ||
| - ``` | ||
| cd heic2png | ||
| chmod +x build.sh |
There was a problem hiding this comment.
This line shouldn't be necessary. Instead of adding this line, you should set the execute bit as part of this PR. I think you can achieve that via git add --chmod=+x build.sh
| ### Compiling on Linux | ||
|
|
||
| 1. Install golang | ||
| - Via apt `apt install golang-go` |
There was a problem hiding this comment.
apt is distribution specific, and historically has really old versions of languages, so I'd rather not include it here at all.
|
|
||
| ### Compiling on Linux | ||
|
|
||
| 1. Install golang |
There was a problem hiding this comment.
The name of the language is actually just Go, not golang.
|
|
||
| [Download the latest release](https://github.com/danbrakeley/heic2png/releases), and then put the exe somewhere in your path. | ||
|
|
||
| ### Compiling on Linux |
There was a problem hiding this comment.
Technically these same steps should work on any OS, not just Linux. Maybe title this ### Building from source?
| chmod +x build.sh | ||
| ./build.sh | ||
| ``` | ||
| 4. Run the script file. |
There was a problem hiding this comment.
If I'm reading this right, I think this step is redundant with step 3?
2 participants
Added a section about how to build for Linux targets. Its probably redundant, since if you are running a Linux setup you probably already know, but it never hurts to be explicit.