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

Support for prebuilt plugin #2864

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

kkawula
Copy link
Collaborator

@kkawula kkawula commented Jan 21, 2025

Closes #2827

Introduced changes

  • Updated publish_plugin workflow to compile plugin for different platforms.

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@kkawula kkawula changed the title Kkawula/2827 support for prebuilt packages in scarb Support for prebuilt plugin Jan 23, 2025
@kkawula kkawula marked this pull request as ready for review January 23, 2025 09:43
Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Please update the description of PR.

.github/workflows/publish_plugin.yml Show resolved Hide resolved
crates/forge/tests/e2e/running.rs Outdated Show resolved Hide resolved
crates/forge/src/new.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@cptartur cptartur left a comment

Choose a reason for hiding this comment

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

@THenry14 can you review the scripts (gh action) part of this?

.github/workflows/publish_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/publish_plugin.yml Show resolved Hide resolved
.github/workflows/publish_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/publish_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/publish_plugin.yml Show resolved Hide resolved
CHANGELOG.md Outdated
Comment on lines 12 to 20
#### Added

- Newly created projects won't require Rust to be installed by default

### `snforge_scarb_plugin`

#### Changed

- Precompiled plugin binaries are now published to the registry for new versions
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's just combine these into single Forge #### Changed entry?

@@ -96,6 +96,7 @@ fn update_config(config_path: &Path, scarb: &Version) -> Result<()> {
set_cairo_edition(&mut document, CAIRO_EDITION);
add_test_script(&mut document);
add_assert_macros(&mut document, scarb)?;
add_allow_prebuilt_macros(&mut document)?;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should always be added. What about older scarb versions that do not support prebuilt macros?

crates/forge/tests/e2e/running.rs Outdated Show resolved Hide resolved
docs/src/appendix/scarb-toml.md Outdated Show resolved Hide resolved
docs/src/getting-started/first-steps.md Show resolved Hide resolved
@cptartur cptartur requested a review from THenry14 January 28, 2025 13:30
@@ -5,33 +5,146 @@ on:
workflow_dispatch:

jobs:
upload-to-registry:
name: Upload plugin to the registry
check-version:
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: technically it's true, but actually you're checking is_version_uploaded or version_exists or just plain upload_requirements. maybe this could be improved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For me, it looks well in the context I use it. Just referencing to outputs

    needs: check-version
    if: needs.check-version.outputs.plugin_uploaded 

.github/workflows/publish_plugin.yml Outdated Show resolved Hide resolved
.github/workflows/publish_plugin.yml Outdated Show resolved Hide resolved
docs/src/appendix/scarb-toml.md Outdated Show resolved Hide resolved
docs/src/appendix/scarb-toml.md Outdated Show resolved Hide resolved
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.

Support for prebuilt packages in scarb
5 participants