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

Bump crystal-ameba/ameba to 1.5.0 #1811

Closed
wants to merge 1 commit into from

Conversation

rsvoboda
Copy link

@rsvoboda rsvoboda commented Aug 5, 2023

This is an automated update of a crystal dependency

WARNING: this requires an upgrade to Crystal. See comments below #1811 (comment)


Bump crystal-ameba/ameba to 1.5.0

Fixes issue with shards install in macOS

Crystal version

Crystal 1.9.2 (2023-07-19)

LLVM: 15.0.7
Default target: x86_64-apple-macosx

Error message:

shards install
...
Building: ameba
Error target ameba failed to compile:
Showing last frame. Use --error-trace for full trace.

There was a problem expanding macro 'macro_5078464816'

Code in /usr/local/Cellar/crystal/1.9.2/share/crystal/src/yaml/serialization.cr:188:7

 188 | {% begin %}
       ^
Called macro defined in /usr/local/Cellar/crystal/1.9.2/share/crystal/src/yaml/serialization.cr:188:7

 188 | {% begin %}

Which expanded to:

 > 75 |
 > 76 |
 > 77 |                   __temp_735 =
                          ^
Error: type must be Ameba::Severity, not (Ameba::Severity | Nil)

make: *** [build] Error 1

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist:

Documentation

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • No updates required.

Copy link
Collaborator

@agentpoyo agentpoyo left a comment

Choose a reason for hiding this comment

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

LGTM, will verify and test before merging.

@rsvoboda
Copy link
Author

Crystal Specs checks fails with

Run USERNAME_ARRAY=($DOCKERHUB_USERNAMES)
Error: Cannot perform an interactive login from a non TTY device
Error: Process completed with exit code 1.

@agentpoyo
Copy link
Collaborator

Crystal Specs checks fails with

Run USERNAME_ARRAY=($DOCKERHUB_USERNAMES)
Error: Cannot perform an interactive login from a non TTY device
Error: Process completed with exit code 1.

Hello @rsvoboda ,

Yes, our build process requires some other authentication with Docker Hub. I'll be doing a checkout and check-in of your source to verify our github actions pass before any merges. Thanks.

@agentpoyo
Copy link
Collaborator

Also to note, we don't officially support Mac for the testsuite but I plan to test both on Mac and our regular build process to ensure there's no other breakage with this crystal module update. Thanks.

@agentpoyo
Copy link
Collaborator

Getting the following error on Linux using our supported crystal version 1.6.0:

pair@cnfdev4:~/workspace/drew/cnf-testsuite-rsvoboda$ shards install
Resolving dependencies
Fetching https://github.com/crystal-community/icr.git
Fetching https://github.com/sija/retriable.cr.git
Fetching https://github.com/icyleaf/halite.git
Fetching https://github.com/cnf-testsuite/tar.git
Fetching https://github.com/icyleaf/totem.git
Fetching https://github.com/vulk/sam.cr.git
Fetching https://github.com/mrrooijen/commander.git
Fetching https://github.com/cnf-testsuite/find.git
Fetching https://github.com/cnf-testsuite/git.git
Fetching https://github.com/cnf-testsuite/docker_client.git
Fetching https://github.com/cnf-testsuite/kubectl_client.git
Fetching https://github.com/cnf-testsuite/kernel_introspection.git
Fetching https://github.com/cnf-testsuite/helm.git
Fetching https://github.com/cnf-testsuite/k8s_netstat.git
Fetching https://github.com/cnf-testsuite/k8s_kernel_introspection.git
Fetching https://github.com/cnf-testsuite/cluster_tools.git
Fetching https://github.com/cnf-testsuite/release_manager.git
Fetching https://github.com/jeromegn/protobuf.cr.git
Fetching https://github.com/crystal-ameba/ameba.git
Fetching https://github.com/icyleaf/popcorn.git
Fetching https://github.com/crystal-lang/crystal-readline.git
Using sam (0.4.0 at 4e3b271)
Using readline (0.1.1 at add6679)
Using icr (0.9.0 at f62bfcd)
Using commander (0.4.0)
Using retriable (0.2.4)
Using popcorn (0.3.0)
Using totem (0.7.0)
Using git (1.0.0)
Using kernel_introspection (0.1.0)
Using docker_client (1.0.0)
Using kubectl_client (1.0.1)
Using cluster_tools (1.0.0)
Using k8s_kernel_introspection (1.0.0)
Using halite (0.12.1)
Using find (0.1.0 at 129096c)
Using tar (0.1.0 at ae9bbea)
Using helm (1.0.1)
Using k8s_netstat (1.0.0)
Using release_manager (0.1.0 at a1d7b35)
Using airgap (0.1.0 at utils/airgap)
Using protobuf (2.3.0)
Installing ameba (1.5.0)
Postinstall of ameba: shards build -Dpreview_mt
Failed postinstall of ameba on shards build -Dpreview_mt:
Dependencies are satisfied
Building: ameba
Error target ameba failed to compile:
Showing last frame. Use --error-trace for full trace.

In src/ameba/rule/style/redundant_begin.cr:126:40

 126 | return unless token.value == Crystal::Keyword::BEGIN
                                    ^----------------------
Error: undefined constant Crystal::Keyword::BEGIN

We'll likely have to bump crystal version as well to jump this far with ameba on the latest version.

@rsvoboda , have you tried using crystal version 1.6.0 on MacOS and ameva 1.3.0 by chance? We tend to update crystal and it's modules on a quarterly basis which the next bump should be around the corner.

Thanks for any input and for submitting this PR, we'll get there.

@rsvoboda
Copy link
Author

Hi @agentpoyo. I used Brew to install crystal (https://formulae.brew.sh/formula/crystal) so that's why I ended up with 1.9.2.
I tried main with crystal https://github.com/crystal-lang/crystal/releases/tag/1.6.0 and compilations goes well.

https://github.com/cncf/cnf-testsuite/blob/main/SOURCE_INSTALL.md mentions that crystal-lang version should be >= (v)1.6.0. Maybe a note saying that future releases of crystal may change some APIs and the recommended version is X.Y.Z.
https://github.com/cncf/cnf-testsuite/blob/main/.crystal-version says 1.5.0, but not sure what tool uses that file.

@agentpoyo
Copy link
Collaborator

Hi @agentpoyo. I used Brew to install crystal (https://formulae.brew.sh/formula/crystal) so that's why I ended up with 1.9.2. I tried main with crystal https://github.com/crystal-lang/crystal/releases/tag/1.6.0 and compilations goes well.

https://github.com/cncf/cnf-testsuite/blob/main/SOURCE_INSTALL.md mentions that crystal-lang version should be >= (v)1.6.0. Maybe a note saying that future releases of crystal may change some APIs and the recommended version is X.Y.Z. https://github.com/cncf/cnf-testsuite/blob/main/.crystal-version says 1.5.0, but not sure what tool uses that file.

Hello @rsvoboda ,

Thanks for confirming that crystal 1.6.0 compiles without issues on MacOS.

That verbiage needs to be updated since future crystal versions could break things before they are fully vetted and tested. I've created a new issue to get crystal upgraded (modules will follow suit as well), you can find that here: https://github.com/cncf/cnf-testsuite/issues/1814

I also believe that .crystal-version file is not used but I'll need to verify.

Thanks again!

@HashNuke
Copy link
Collaborator

HashNuke commented Aug 21, 2023

I can confirm that .crystal-version is not being updated. We can update it if it helps. I'll share the command I use to install a specific version of crystal below.

# NOTE: Linux only
curl -fsSL https://crystal-lang.org/install.sh | sudo bash -s -- --version=1.6

The command above is from the Crystal site and installs the latest 1.6.x. You can replace it with 1.9 and it will work as expected.

@HashNuke
Copy link
Collaborator

@agentpoyo Just to confirm, we need to hold off on merging this PR. I looked up ameba's shard.yml and it requires crystal 1.9.

@rsvoboda The testsuite is always pinned to a specific minor version of Crystal as a requirement. For Mac, to install a specific version, the crystal website mentions using the github releases as the only way. Below is the link to the 1.6.2 release.

https://github.com/crystal-lang/crystal/releases/tag/1.6.2

@taylor taylor marked this pull request as draft April 1, 2024 20:41
Fixes issue with shards install in macOS

Signed-off-by: Rostislav Svoboda <[email protected]>
@rsvoboda
Copy link
Author

Tried to rebase to the latest, compilation fails.

Not worth the effort as concrete version of Crystal is needed

@rsvoboda rsvoboda closed this Sep 24, 2024
@kosstennbl
Copy link
Collaborator

The issue is still very relevant, being discussed on community meetings and in the scope of #2156. I'm planning to gather knowledge about all needed changes to make this update.

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.

5 participants