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

Update Crystal v1 plugin #3541

Merged
merged 5 commits into from
Jul 22, 2021

Conversation

Blacksmoke16
Copy link
Contributor

@Blacksmoke16 Blacksmoke16 commented Jul 1, 2021

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh tests/unit?

  • Clarifies the documentation that the crystal-build-options are passed to the shards build command
  • Add some additional build dependencies
    • Biggest one being git. Without it shards fails to install dependencies
  • Consolidate build step into a single shards build command
  • Update the build flag to use --without-development

Manually tested by loading in the changes as a local plugin and build a Crystal shard via snap

Fixes Blacksmoke16/oq#81

@mamantoha
Copy link
Contributor

@Blacksmoke16 nice. With these changes will be possible to build a package with --ignore-crystal-version option.

@Blacksmoke16
Copy link
Contributor Author

@mamantoha This PR mainly fixes the case where you're building a project (lib) that doesn't have a shard.lock. You can already include that flag, but because git was missing it would fail to clone the dependent shards.

@mamantoha
Copy link
Contributor

@Blacksmoke16 yeah, I saw.

Also previously it runs 2 commands:

  • shards install --production
  • shards build --production

And additional options can be passed only to the second command. This didn't allow me to build snap with --ignore-crystal-version.

So, thanks for fixing this :)

@mamantoha
Copy link
Contributor

@Blacksmoke16 btw, if you want to add additional packages you can use build-packages .
See https://github.com/mamantoha/zipstream/blob/master/snap/snapcraft.yaml#L34 for reference.

@Blacksmoke16
Copy link
Contributor Author

@mamantoha Oh nice, that's good to know. My thinking is that the plugin should include these base packages so that anything you do with the stdlib works. Otherwise you'd prob run into linking issues and need to figure out what lib you need to resolve that. build-packages deff seems to make more sense if your shard is using external/non-standard libs.

On the other hand I could also see an argument for the built in packages to only require the ones absolutely required. Then require the end user to specify additional ones, e.g. if you require "yaml" you need to add libyaml-dev. However I decided against it as it didn't seem like a big deal to me.

@sergiusens
Copy link
Collaborator

Very nice

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2021

Codecov Report

Merging #3541 (575b7ab) into master (8649ced) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3541      +/-   ##
==========================================
- Coverage   91.09%   91.09%   -0.01%     
==========================================
  Files         276      276              
  Lines       18917    18916       -1     
==========================================
- Hits        17233    17232       -1     
  Misses       1684     1684              
Impacted Files Coverage Δ
snapcraft/plugins/v1/crystal.py 90.00% <ø> (-0.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8649ced...575b7ab. Read the comment docs.

@Blacksmoke16
Copy link
Contributor Author

Not sure why CI is failing. I deff signed the CLA.

@Blacksmoke16
Copy link
Contributor Author

@sergiusens I think I figured out the CLA issue. I got an email the other day that You have been added to contributor-agreement-canonical. My Launchpad account's email was different than my Github. I've since added my other emails to my Launchpad account so hopefully that'll resolve that.

@sergiusens
Copy link
Collaborator

That sounds like the root of @Blacksmoke16 ... I was very busy this week and I seem to have missed my comment about maybe reauthing your commits to match your email address, your solution is a lot better :-D

@sergiusens sergiusens merged commit 868134f into canonical:master Jul 22, 2021
@Blacksmoke16 Blacksmoke16 deleted the crystal-plugin-updates branch July 22, 2021 14:53
@Blacksmoke16
Copy link
Contributor Author

Blacksmoke16 commented Aug 1, 2021

@sergiusens Is it expected that my builds on snapcraft.io/build still are using 4.8.3 versus the new 5.0.0 release? This is causing the build to continue to fail due to this change only being in the new release.

This will be transparently supported on snapcraft.io/build and Launchpad by using Snapcraft's 4.x channel track.

Does this mean that that service will always be using 4.x flow or that it'll only use that flow IF base is missing or set to core?

EDIT: NVM, just saw via https://snapcraft.io/snapcraft that it's not in the stable channel yet, so probably just need to wait some more.

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.

Snap failed to build
4 participants