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 ws command line interface. #23

Merged
merged 17 commits into from
Aug 31, 2022
Merged

Add ws command line interface. #23

merged 17 commits into from
Aug 31, 2022

Conversation

zw963
Copy link
Contributor

@zw963 zw963 commented Aug 13, 2022

This commit including following changes:

  1. Add src/ws.cr, it used to build ws command, it not required by default.
  2. Add a new github action, when we are release a new version use git tag, it will built a packed version alpine static binary which can be download directly on release page, we probably should support download macOS version, but i need investigate how to do it, because i never use Mac.
  3. some Improvement.

Code probably still optimizing, but it works!

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Just a few small suggestions, but this looks great! Thank you.

.github/workflows/alpine_x86_64_release.yml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/ws.cr Outdated Show resolved Hide resolved
zw963 and others added 6 commits August 14, 2022 12:01
Co-authored-by: Jeremy Woertink <jeremywoertink@gmail.com>
Co-authored-by: Jeremy Woertink <jeremywoertink@gmail.com>
Co-authored-by: Jeremy Woertink <jeremywoertink@gmail.com>
@zw963
Copy link
Contributor Author

zw963 commented Aug 14, 2022

Hi, @jwoertink , i try improve banner usage for ws again, please help on check english. : )

And bump a new version to fix some error when pacakge and avoid flush ours discussion.

One more question:

I found when i add following code into shard.yml, the ws binary was not build when wordsmith
add as a lib dependency, and installed as shards install.

diff --git a/shard.yml b/shard.yml
index 014b24d..b7dd167 100644
--- a/shard.yml
+++ b/shard.yml
@@ -5,6 +5,10 @@ authors:
   - Paul Smith <paulcsmith0218@gmail.com>
   - Flinn Mueller <theflinnster@gmail.com>
 
+targets:
+  ws:
+    main: src/ws.cr
+
 crystal: ">= 1.4.0"

This is so wired, because i want to remove this diff because i thought ws will be build if i add a targets item into shard.yml.
But it seem like build ws never happen, but, i check https://github.com/luckyframework/avram/blob/main/shard.yml#L8-L10
it seem like works, i just want to confirm, if we want install any prebuild binary, we must add a postinstall item in shard.yml?

Thank you.

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

I've added a few new comments for the new updates.

.github/workflows/apple_darwin_release.yml Outdated Show resolved Hide resolved
.github/workflows/alpine_x86_64_release.yml Show resolved Hide resolved
shard.yml Outdated Show resolved Hide resolved
shard.yml Outdated Show resolved Hide resolved
@zw963
Copy link
Contributor Author

zw963 commented Aug 15, 2022

I test the mac version ws precompile binary, it works.

For those tag, because i create tag v0.3.1 and v0.3.2 on my fork, should i remove those tag before merge?

@zw963
Copy link
Contributor Author

zw963 commented Aug 18, 2022

I consider if the name of ws a litter shorter? though, i search on my Arch linux package manager with /usr/bin/ws with no result.

I don't know if there is a command told ws in mac anyway.

@jwoertink
Copy link
Member

I searched apt and didn't see any ws programs, so we may be fine. As for the tags, I'm not sure how that works. If the merge carries them over, then yeah, they need to go away. I can't imagine that's the case though because I'd think that could lead to a security issue of people just making random tags in commits 😬

@zw963
Copy link
Contributor Author

zw963 commented Aug 18, 2022

I searched apt and didn't see any ws programs, so we may be fine. As for the tags, I'm not sure how that works. If the merge carries them over, then yeah, they need to go away. I can't imagine that's the case though because I'd think that could lead to a security issue of people just making random tags in commits grimacing

I guess the tag should never be merged in a PR.

And for the tag, it means when maintainer create a new valid version tag like this: v0.0.3 which start with v, the builder workflow will create a binary, anyway, i consider we don't need build binary each when push code into base, right?

@zw963 zw963 requested a review from jwoertink August 31, 2022 05:19
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

It looks good to me 👍 I guess we can just merge this in and give it a shot. Thanks for putting this together!

@jwoertink jwoertink merged commit 438eef7 into luckyframework:main Aug 31, 2022
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.

2 participants