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

Add ability to specify custom flags #2

Merged
merged 9 commits into from
Sep 24, 2019
Merged

Add ability to specify custom flags #2

merged 9 commits into from
Sep 24, 2019

Conversation

crazy-max
Copy link
Contributor

@crazy-max crazy-max commented Sep 22, 2019

Fixes #1 and also use @actions/exec instead of child_process dep

Use @actions/exec instead of child_process
@svenstaro
Copy link
Owner

Awesome! I like the usage of @actions/exec! Could you also add the for the strip part in order to solve #1 in its entirety?

@svenstaro
Copy link
Owner

Perhaps you could also add a new complex example using these args to the end of the README?

@crazy-max
Copy link
Contributor Author

crazy-max commented Sep 23, 2019

Could you also add the for the strip part in order to solve #1 in its entirety?

I wonder what's the purpose of strip here ? UPX already handles this and this adds to this action another layer that could be unexpected for the user.

Perhaps you could also add a new complex example using these args to the end of the README?

Yes of course

@svenstaro
Copy link
Owner

In my tests, no stripping is done by UPX and the binary ends up a lot bigger without stripping.

@crazy-max
Copy link
Contributor Author

crazy-max commented Sep 23, 2019

@svenstaro I have just checked, you right! So I think we could add an input to enable/disable strip command because for Windows binaries strip could lead to unexpected behavior.

@svenstaro
Copy link
Owner

So far in my tests, strip was always available in GitHub Actions and is working as intendend.

@crazy-max
Copy link
Contributor Author

I talked about final result when a Windows executable is stripped.
For example if I execute strip on innosetup-5.6.1-unicode.exe the stripped executable is broken.

@svenstaro
Copy link
Owner

I use this action here and it seems fine.

Add strip and strip_args input
@crazy-max
Copy link
Contributor Author

@svenstaro Ok I have made some changes ;)

README.md Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

Good changes so far, but I'm still missing the second example for the README with the args in use.

src/action.ts Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

Seems good to me. Once the README is expanded this seems good to merge.

@crazy-max
Copy link
Contributor Author

@svenstaro I have updated README with a complete example of the available inputs.

@svenstaro
Copy link
Owner

I'd prefer a second example. One for the simple case and one for the complex case. For reference, see https://github.com/svenstaro/upload-release-action

@crazy-max
Copy link
Contributor Author

Done 🙂

@svenstaro
Copy link
Owner

Great stuff!

@svenstaro svenstaro merged commit e3284c3 into svenstaro:master Sep 24, 2019
@svenstaro
Copy link
Owner

Released as 1.1.0.

@crazy-max
Copy link
Contributor Author

Yeah 🚀 🎉

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.

Add ability to specify custom flags for upx and strip
2 participants