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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - TY-2333 - Include flutter plugin in the CI #107

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - TY-2333 - Include flutter plugin in the CI #107

wants to merge 1 commit into from

Conversation

x3ro
Copy link
Contributor

@x3ro x3ro commented Feb 3, 2022 •
edited by jira bot
Loading

@github-actions github-actions bot added the work-in-progress The author is still working on this PR label Feb 3, 2022
@x3ro x3ro force-pushed the TY-2333-include-flutter-in-ci branch 14 times, most recently from 1141087 to 2dfbabd Compare February 9, 2022 08:22
acrrd
acrrd reviewed Feb 9, 2022
View reviewed changes
.github/actions/setup-job/action.yml Outdated
just-version: '0.10.5'
just-version: ${{ env.JUST_VERSION }}

- name: Restore Rust toolchain cache
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not restoring the toolchain cache it cannot be called before the toolchain is there.

.github/actions/setup-job/action.yml Outdated
if: inputs.rust != 'false'
with:
path: |
target/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already cached by rust-cache.
We can move this call at the end together with the other rust parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmh, I'm going to double-check then. I saw some build times cut in half with this caching added, but maybe that was just a fluke.

Copy link
Contributor

Choose a reason for hiding this comment

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

rust-cache does not keep our own build code: Swatinem/rust-cache#37
The code is not versioned for the cache so this can create problem on the long run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but the cache here is bound to the Cargo.lock, which changes quite frequently for us. So for branches I definitely caching target/ is desirable, though one could think about disabling it in bors / main.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, I'm going to revert that part to the state of before for now. No need to look into this in detail at the moment.

.github/workflows/ci.yml
@@ -21,7 +21,7 @@ permissions:

jobs:
dev-ci:
uses: xaynetwork/xayn_discovery_engine/.github/workflows/ci_reusable_wf.yml@main
uses: ./.github/workflows/ci_reusable_wf.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this really work now? When we started doing this it wasn't possible. Would be very nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please also do this change in the bors ci?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least for me, on my branch, this has been working fine :)

justfile Show resolved Hide resolved
justfile
@@ -1,5 +1,6 @@
# We import environment variables from .env
set dotenv-load := true
set shell := ["bash", "-uc"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we set this? If we set this do we still have to specify #!/usr/bin/env sh in other jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the shell that all commands are run with. I assume you're referring to this:

dart-deps:
    #!/usr/bin/env sh
    ....

This is what Just calls a "shebang recipe", and it's always preceded by the shell you want to run it with.

Shebang recipe bodies are extracted and run as scripts [...].

So long story short: I set bash so I can use =~ and [[, but this does not affect the recipes you're referring to, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if with this definition all commands are implitly shebang recipe for every task or if we still have to it to avoid to have to put \ at the end of a line to have all the commands run in the same shell and not a new shell per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that is unfortunately not how it works, I think 🤔 But I will double-check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the docs:

Recipes without an initial shebang are evaluated and run line-by-line [...]

So yeah, if we'd want to remove the backslashes we'd have to include the shebang for every recipe.

justfile Outdated Show resolved Hide resolved
@x3ro x3ro force-pushed the TY-2333-include-flutter-in-ci branch from 2dfbabd to 8574644 Compare February 9, 2022 08:56
@x3ro x3ro marked this pull request as ready for review February 9, 2022 08:56
@x3ro x3ro removed the work-in-progress The author is still working on this PR label Feb 9, 2022
@github-actions github-actions bot added the ready-for-review The PR can be reviewed label Feb 9, 2022
@x3ro x3ro requested a review from acrrd February 9, 2022 09:05
acrrd
acrrd approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@acrrd acrrd left a comment

Choose a reason for hiding this comment

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

Can you please do the same change you did in ci.yml to bors_ci.yml so we don't have to do a one line pr for that.

@x3ro x3ro force-pushed the TY-2333-include-flutter-in-ci branch from 8574644 to 36195de Compare February 9, 2022 10:15
@x3ro
Copy link
Contributor Author

x3ro commented Feb 9, 2022

bors merge

@x3ro x3ro removed the ready-for-review The PR can be reviewed label Feb 9, 2022
bors bot pushed a commit that referenced this pull request Feb 9, 2022
@x3ro
TY-2333 - Include flutter plugin in the CI (#107)
2ef8fab 
[TY-2333]

[TY-2333]: https://xainag.atlassian.net/browse/TY-2333?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

Co-authored-by: Lucas Jenss <lucas.jenss.external@xayn.com>
@bors
Copy link

bors bot commented Feb 9, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title TY-2333 - Include flutter plugin in the CI [Merged by Bors] - TY-2333 - Include flutter plugin in the CI Feb 9, 2022
@bors bors bot closed this Feb 9, 2022
@bors bors bot deleted the TY-2333-include-flutter-in-ci branch February 9, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acrrd acrrd acrrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@x3ro @acrrd