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

Proposal: GitHub Actions Workflow & Unix-style Piping Support #30

Open
1 of 2 tasks
0bCdian opened this issue Feb 13, 2025 · 18 comments
Open
1 of 2 tasks

Proposal: GitHub Actions Workflow & Unix-style Piping Support #30

0bCdian opened this issue Feb 13, 2025 · 18 comments
Labels
Proposal Discussing the implementation of features

Comments

@0bCdian
Copy link

0bCdian commented Feb 13, 2025

Hi! I've been checking out this project while learning Go, and I think it's awesome! I want to contribute with a couple of features that could make it even more flexible and easier to use.

Proposed Contributions

1. GitHub Actions Workflow for Automated Binary Releases

  • Completed

I’d like to set up a GitHub Actions workflow to compile binaries for different platforms and attach them to the releases page automatically. This way, users can just grab a prebuilt binary instead of having to build from source.

Why?

  • Users get instant access to up-to-date binaries.
  • No need to manually build if they just want to use the tool.
  • Makes installation a breeze and encourages more people to try it out.
  • You could add a -bin package to the aur for example as many other packagers do.

2. Support for Unix-style Piping (stdin/stdout Processing)

  • Completed

It would be super useful if the tool could read images from stdin and write to stdout, making it possible to compose commands in a pipeline like traditional Unix tools.

Example Use Case:

curl -s https://somerandomimage.png | gowall convert -t gruvbox - | gowall draw ... > ./output.png

Why?

  • Adds flexibility for scripting and automation.
  • No need to create temporary files just to process images.
  • Feels more natural in a Unix/Linux environment.

Implementation Approach

  • Update the code to accept input from os.Stdin when - is passed as a filename.
  • Write processed images to os.Stdout if no output file is specified.
  • Update the docs to explain how to use these new features.

I think these changes would make the project a lot more powerful, especially for those who love scripting and automation. Let me know what you think—I’d love to get started on this!

@Achno
Copy link
Owner

Achno commented Feb 13, 2025

Hello @0bCdian thank you for the kinds words :)

Lets start addressing your proposals:

First proposal

  1. GitHub Actions Workflow for Automated Binary Releases

This is something i 100% want to do, albeit it seems simple at first there is a reason why i haven't yet implemented that.
I was already testing github actions out locally with https://github.com/nektos/act and faced an issue with the webp library.

Under the hood it uses CGO and to say the least its biting me in the *** now. Check this linux build error : here for more information. When i tried to automate the binary releases this lib caused a lot failures when compiling for a lot of architectures. Not only this its connected to other issues check : #18

I was waiting to release v.0.2.0 so i can start working on this, so i have a stable version and can start experimenting in the next version about removing/replacing the library.

In conclusion for the first proposal

This is very complex and i think its best to tackle it myself since it involves big decisions (removing/changing libs)
and it was next in my todo list anyway.


Second proposal

  1. Support for Unix-style Piping (stdin/stdout Processing)

You must be reading my mind because this is also something i also want in the project.

Image
Before i give you the green light to start working on this, lets talk about the implementation more.

1) First issue

Instead of using - where its used for flags i was thinking something like +. Why? Essentially i want something that the shell nor the cli framework does not intercept.

Take a look at this "bug" from a few months ago here. Gowall has a feature where it can convert all files under a path. To do that it uses the # delimiter

 gowall convert ~/Pictures/# -t catppuccin // converts all images under that folder

In that example the shell for some reason intercepted the # delimiter and the user had to put it with quotes to signify a string "~Pictures/#" . I fear that the shell will intercept - the same way. For that reason we need a different symbol or a new approach.

Building blocks

Since i was preparing to create this feature eventually i have refactored and added some functionality that would help.

type ProcessOptions struct {
	SaveToFile bool   // Whether to save the processed image to file
	OutputExt  string // Optional output extension to override the original
	OutputName string // Optional outputName
}

func ProcessImg(imgPath string, processor ImageProcessor, theme string, opts ...ProcessOptions) (string, *image.Image, error) { ... }

The ProcessImg function takes some options, one of these is the SaveToFile. If this is false it wont create a file so you can just get the *image.Image from the function and pass it to the next step of the pipeline without temporary files.

In conclusion for the second proposal

You can start by forking and playing around with it, but we must clarify the implementation before we run into issues.

@Achno Achno added the Proposal Discussing the implementation of features label Feb 13, 2025
@0bCdian
Copy link
Author

0bCdian commented Feb 13, 2025

Thanks for taking the time to read through the proposal! And of course, I will let you know in advance of any progress and or / changes that I implement on my side, I'm 100% on board with having a clear communication channel so we can implement these features as smoothly as possible. We can use this issue for that, or any other channel you prefer.

Having said that I have a few things to comment:

This is something i 100% want to do, albeit it seems simple at first there is a reason why i haven't yet implemented that.
I was already testing github actions out locally with https://github.com/nektos/act and faced an issue with the webp library.

Under the hood it uses CGO and to say the least its biting me in the *** now. Check this linux build error : chai2010/webp#48 for more information. When i tried to automate the binary releases this lib caused a lot failures when compiling for a lot of architectures. Not only this its connected to other issues check : #18

I was waiting to release v.0.2.0 so i can start working on this, so i have a stable version and can start experimenting in the next version about removing/replacing the library.

In conclusion for the first proposal
This is very complex and i think its best to tackle it myself since it involves big decisions (removing/changing libs)
and it was next in my todo list anyway.

Excuse me if it sounds silly, but have you tried as a temporary workaround dockerizing the build process and using volumes to output the binary? I have used this technique before for other use cases and it works great. I have yet to test this approach but I can try it on my side and report the results.

  1. First issue
    Instead of using - where its used for flags i was thinking something like +. Why? Essentially i want something that the shell nor the cli framework does not intercept.

Take a look at this "bug" from a few months ago #17 (comment). Gowall has a feature where it can convert all files under a path. To do that it uses the # delimiter

gowall convert ~/Pictures/# -t catppuccin // converts all images under that folder
In that example the shell for some reason intercepted the # delimiter and the user had to put it with quotes to signify a string "~Pictures/#" . I fear that the shell will intercept - the same way. For that reason we need a different symbol or a new approach.

Hmmm I have never encountered this issue myself with other tools that use - to signify stdout, but I guess it depends on the shell, I'll have to dig deeper into this. In the worst case, I think we can simply implement a --stdout flag or "simply" not pass anything, but it would require reworking the current implementation, changing from the output being implicit to a specific predefined directory to the more common approach of being explicit about the output with an -o or --output flag. Of course, I still need to dig deeper into the codebase to make a more informed opinion about the subject, but let me know what you think!

@Achno
Copy link
Owner

Achno commented Feb 13, 2025

Excuse me if it sounds silly, but have you tried as a temporary workaround dockerizing the build process and using volumes to output the binary?

You could probably get this working as long as you followed this.

But i'm a bit confused on how to integrate that with goreleaser github action, not having a lot of experience with CI/CD in general.

What i want to it just throw out the old webp lib and replace it with this pure go webp encoder and just use the goreleaser github action and automate everything.

Hmmm I have never encountered this issue myself with other tools that use - to signify stdout

Me either, its really strange and obscure. Can you point some of these tools out so i can see how they implement this? I dont really think its a major cause of concern though.

I think we can simply implement a --stdout flag or "simply" not pass anything

I think im going to rule out the not pass anything way. Between the - and an --stoud i would still prefer the - way.

@0bCdian
Copy link
Author

0bCdian commented Feb 13, 2025

You could probably get this working as long as you followed chai2010/webp#48 (comment).

But i'm a bit confused on how to integrate that with goreleaser github action, not having a lot of experience with CI/CD in general.

What i want to it just throw out the old webp lib and replace it with this pure go webp encoder and just use the goreleaser github action and automate everything.

I will toy around with it and if I'm successful I'll report back with a draft and explain the process. I'm quite familiar with ci/cd so it is not a problem for me to get my hands dirty with this issue. Btw, which platforms would you like to target?

Also here are some tools that I use regularly that use the - to signify stdin or stdout
satty and grim

@Achno
Copy link
Owner

Achno commented Feb 13, 2025

Btw, which platforms would you like to target?

goos:
      - windows
      - darwin
      - linux
goarch:
      - amd64
      - arm64

which are the default of goreleaser essentially

I'll report back with a draft and explain the process

Thank you for the help :)

@0bCdian
Copy link
Author

0bCdian commented Feb 14, 2025

Hey! I've come to report good news, after some failed attempts and a lot of reading I have successfully built the binary for all the targets you needed:

Image

https://github.com/0bCdian/gowall/actions/runs/13325955334

The workflow is not yet complete, I would have to make now the actual part that pushes the binaries to the latest release. But that part is trivial.

I used zig cc to cross-compile the webp library to the desired targets since CGO was not working in Linux and Windows for arm64.

sources:

@0bCdian
Copy link
Author

0bCdian commented Feb 14, 2025

I can get the whole working workflow done by today late evening (I'm kinda busy today) and make a pr if you give the green light.

@Achno
Copy link
Owner

Achno commented Feb 14, 2025

@0bCdian

since CGO was not working in Linux and Windows for arm64.

That was where i was hitting a wall in the past. Pretty smart to use Zig, did not think about that.

As for the workflow i will use it to generate the binaries and put them in the v0.2.0 release. I noticed also that it took 7mins for this workflow to complete.

I have decided to just change the webp lib completely so there is no need to continue this workflow, the lib still causes problems and i want it out. Plus it will cut the workflow time by a lot with the new replacement.

But this workflow is very useful for the big feature of the next release OCR. Im 100% using a lib with CGO in it so if normal compilation does not work this zig cross compilation will be the saviour. Thanks so much couldn't have figured that out by myself.

You can start on the pipe support whenever you want :)

@0bCdian
Copy link
Author

0bCdian commented Feb 14, 2025

There are some rough edges and things I can improve in the workflow, but I wanted to get out of the way compilation part first before doing the whole thing. I'll start digging into the code base and research to make some drafts and proposed changes for the stdin/out stuff. If you need any help with the workflow or anything else I'm glad to help!

@Achno
Copy link
Owner

Achno commented Feb 14, 2025

No worries and above all no pressure, take your time and only work on it when you feel like it, it's not anything urgent by any means.

@Achno
Copy link
Owner

Achno commented Feb 20, 2025

Added progress indicators to the proposal and finished the GitHub Actions Workflow for Automated Binary Releases part in 02999f7.

When a commit with a tag gets pushed it will trigger the workflow and build the binaries for the package.

@0bCdian
Copy link
Author

0bCdian commented Feb 21, 2025

I'm sorry I still haven't made progress with the other proposal, I have been busy lately with my job search / personal stuff, but this weekend I'll have more time to sit down and code.

@Achno
Copy link
Owner

Achno commented Feb 21, 2025

@0bCdian You don't have anything to apologize for man don't worry,

A job search is much more important than contributing to an open source project, as i said before only contribute only when you have time and you enjoy doing it. Focus on that job hunting, i know its a pain to get a job in this market in Europe and i myself will have to start looking for some internships soon when my semester ends. All the best in your future endeavors.

@0bCdian
Copy link
Author

0bCdian commented Feb 21, 2025

@Achno Thanks for understanding man, I appreciate it. I'll try to have some draft done by Sunday tops.

@0bCdian
Copy link
Author

0bCdian commented Feb 23, 2025

Hi @Achno, I'm passing by to let you know how it's going. I'm still in the discovery phase and investigating popular CLI tools to see what decisions they made when dealing with streams and pipes.

Just as a first draft or proposal, we can do the following:

In regards to receiving data via stdin we could have two approaches:

  1. Treat - as a special pseudofilename gowall - convert -t ...... This is the style that ImageMagick uses, for example:

STDIN, STDOUT, and file descriptors
Linux and Windows permit the output of one command to be piped to the input of another. ImageMagick permits image data to be read and written from the standard streams STDIN (standard in) and STDOUT (standard out), respectively, using a pseudo-filename of -. In this example, we pipe the output of magick to the display program:

magick logo: gif:- | magick display gif:-

The second explicit format "gif:" is optional in the preceding example. The GIF image format has a unique signature within the image, so ImageMagick's display command can readily recognize the format as GIF. The magick program also accepts STDIN as input in this way:

magick rose: gif:- | magick - -resize "200%" bigrose.jpg

In summary, we always read from stdin if no filename is passed directly, and - represents both stdin and stdout.

  1. We read from stdin when the filename is empty, but we specify stdout via flags, like --stdout. This approach is more explicit and robust in the sense that we don't risk using a potential metacharacter from a shell.

I personally vote for 1 as it's more neat and compact, and it's in the style of many popular tools, but if you think it's too risky, we can go the other route, adding verbosity for explicitness and potentially a more robust approach, but I honestly don't think we are gaining anything with 2), many popular cli tools have been using style 1) with no issues for years, so I don't see why we could bump into such issues.

Hierarchy of behavior:

  • CLI > Config file
  • Filepath > stdin

So if we do something like:

cat someimage.png | gowall convert anotherImage.webp

anotherImage.webp takes precedence over the stdin stream file, and the file is output to the configured by default or whatever the user put in their config file.

Another example:

cat someimage.png | gowall convert anotherImage.webp - #style1
cat someimage.png | gowall convert anotherImage.webp --stdout #style2

anotherImage.webp still takes precedence over the stdin stream file, but the output destination in the configuration file is overriden in favor of stdout.

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

Hey @0bCdian

Okay so lets go with this like ImageMagick did :

cat image.jpg | magick - -r 200% -        #Imagemagick 

First - (input) → Read from stdin
Second - (output) → Write to stdout

cat image.jpg| gowall invert -  - |  gowall convert -  -t catppuccin

First - (input) → Read from stdin in gowall invert
Second - (output) → Write to stdout in gowall invert

in this gowall convert - -t catppuccin we only specified the first - so it only accepts from stdin and DOES NOT WRITE TO STDOUT, rather it saves to ~/Pictures/gowall/... or whatever you have specified in the config.yml.

gowall invert img.png  - |  gowall convert -  -t catppuccin

The first gowall invert img.png - just outputs to stdout because the 1st arg is an image path while the second arg is -
The second gowall convert - -t catppuccin just takes the stdin because the 1st arg is - and saves to ~/Pictures/gowall/...

Try to not touch the switch statements in the cmd folder instead just add a new case where it checks if one of theargs is -

@0bCdian
Copy link
Author

0bCdian commented Feb 24, 2025

Perfect! I'll start playing with my local version. Btw I see you didn't mention the case where no file input is given, so an implied stdin input:

gowall invert img.png - | gowall convert -t catppuccin

This would take whatever is in the stdin and output it to the default directory/the one specified in the config file.

You don't like implicitly consuming from stdin or just forgot to add it in your comment?

In unrelated news, I have my second interview in a few days for a DevOps position 😁!

@Achno
Copy link
Owner

Achno commented Feb 24, 2025

@0bCdian

Btw I see you didn't mention the case where no file input is given, so an implied stdin input:

Yea i don't want this , the user must explicitly use - to indicate both stdin and stdout.
You also won't have to touch the other cases in the cmd folder because they already throw an error if nothing is given as input and i do want this behavior changed.

In unrelated news, I have my second interview in a few days for a DevOps position 😁!

Good job man, you got this! Personally since im still in my 3rd year out of 5 years in Uni, i still haven't yet decided what exactly i want to be but i'm studying kubernetes and a lot of networking just for fun and to build a good homelab :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Discussing the implementation of features
Projects
None yet
Development

No branches or pull requests

2 participants