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

Windows Actions CI - use $ErrorActionPreference for bundler install step #1531

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

MSP-Greg
Copy link
Contributor

At present, all Actions Windows Rubies have Bundler installed. If an update is needed, doing so with gem update --system is probably a better idea.

bundle config --local path .bundle/gems
bundle config --local without docs lint coverage
# warning output can confuse Powershell in CI, (multi-line scripts & exit codes)
$env:RUBYOPT = '-W0'
Copy link
Contributor

@slonopotamus slonopotamus Jan 30, 2020

Choose a reason for hiding this comment

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

What if bundler call is split into a separate step? Does it also fail to print warnings?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mojavelinux and what's the point storing gems in .bundle/gems? Ain't everything supposed to work regardles of gems location? Whole CI VM will be dropped anyway after build is finished.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@slonopotamus

What if bundler call is split into a separate step? Does it also fail to print warnings

It's not about it failing to print warnings. It's about warnings causing the step to fail when they shouldn't.

There's been discussion in RubyGems about this, in that warnings are printed when a user has no control over the gem in question. The idea was to change the warnings to only show when the gem is built, not when it is installed.

Copy link
Member

Choose a reason for hiding this comment

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

The use of path is no about caching. It's about testing the installation of the CLI correctly and in isolation. Without specifying a path, the CLI location test has to be partially faked.

@slonopotamus
Copy link
Contributor

Oh, you mean it exits with 1 on warnings? That's weird.

@MSP-Greg
Copy link
Contributor Author

@slonopotamus

Re gem location (.bundle/gems), that can be cached in Actions so they aren't downloaded all the time. But, I think the caches empty after seven days, so if there aren't a lot of commits/PR's...

I didn't set up caching. Using .bundle/gems can be helpful for people working locally.

@MSP-Greg
Copy link
Contributor Author

@slonopotamus

Oh, you mean it exits with 1 on warnings? That's weird.

Yes, see some of the previous actions jobs.

Quite the PITA. I've had the issue before (and also on Appveyor). As I mentioned in ruby/setup-ruby#13, I do not have the issue running the commands locally. Someday I have to track the issue down re PowerShell configuration...

@slonopotamus
Copy link
Contributor

I would avoid caching unless there's a strong need to improve build times, because it can hide bugs when gems are available because they were cached and not because there's a dependency on them.

@mojavelinux
Copy link
Member

all Actions Windows Rubies have Bundler installed.

Oh, I didn't realize that. Then no need for this step. I was just following the README https://github.com/eregon/use-ruby-action#caching-bundle-install (which I now see says that Bundler is already there).

@mojavelinux
Copy link
Member

mojavelinux commented Jan 30, 2020

Using .bundle/gems can be helpful for people working locally.

It also forces Bundler to install the CLI (the bin scripts) for the current gem, which then allows me to test the CLI properly.

@mojavelinux
Copy link
Member

But why do we get warnings when bundle config is used and not when the CLI flags like --path are used? I don't get it.

bundle config --local path .bundle/gems
bundle config --local without docs lint coverage
# warning output can confuse Powershell in CI, (multi-line scripts & exit codes)
$env:RUBYOPT = '-W0'
Copy link
Member

Choose a reason for hiding this comment

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

Sweet trick!

Copy link
Member

Choose a reason for hiding this comment

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

Except don't we need to unset it afterwards so it doesn't remain set while running tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except don't we need to unset it afterwards so it doesn't remain set while running tests?

No. Big difference between Azure Pipelines / GitHub Actions and Travis / AppVeyor. In Actions, changes to the environment do not affect subsequent steps, unless one uses something like:

echo '::set-env name=JOBS::'-j$((1 + $(nproc --all)))

Sometimes it's helpful, often not...

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the info!

@mojavelinux
Copy link
Member

So my question still stands, what is the warning? And how is this different from the warning when --path is used?

[DEPRECATED] The `--path` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set path '.bundle/gems'`, and stop using this flag

@slonopotamus
Copy link
Contributor

But why do we get warnings when bundle config is used and not when the CLI flags like --path are used? I don't get it.

Could you once again give a link to workflow run with warnings? AFAIR, there was something about prawn-tables, but not about config options you use.

@mojavelinux
Copy link
Member

mojavelinux commented Jan 30, 2020

there was something about prawn-tables

That was just the last message logged. It has nothing to do with this situation.

But why do we get warnings when bundle config is used and not when the CLI flags like --path are used?

Here's an example:

https://github.com/asciidoctor/asciidoctor-pdf/runs/416845301

(view the raw log)

@slonopotamus
Copy link
Contributor

slonopotamus commented Jan 30, 2020

@mojavelinux have you tried splitting that in separate steps so it becomes clear which exact comand returns 1?

@mojavelinux
Copy link
Member

mojavelinux commented Jan 30, 2020

Here's what I'm saying. If I run this it works fine:

bundle --jobs 3 --retry 3 --path .bundle/gems --without docs lint coverage

If I run this, it fails:

bundle config --local path .bundle/gems
bundle config --local without docs lint coverage
bundle --jobs 3 --retry 3

Why does it die in one case and not in the other? It's exactly the same set of instructions. The only difference is that there are 3 commands instead of 1.

Btw, the same thing happens if I write directly to .bundle/config instead of using bundle config.

I get that using -W0 is the fix. I just want to know why it is needed in the second case.

@mojavelinux
Copy link
Member

The command that fails is bundle --retry 3 --jobs 3. If we split in separate steps, maybe that also solves it. I guess we could try.

@MSP-Greg
Copy link
Contributor Author

Have at it. As I've mentioned, I could not repo this locally. See
ruby/setup-ruby#13 (comment)

@mojavelinux
Copy link
Member

I've been able to confirm that using separate steps also solves the problem. I'm going to go with that approach instead. Thanks for helping us troubleshoot this, @MSP-Greg! Much appreciated!

@MSP-Greg
Copy link
Contributor Author

Give me a second, running CI now...

@MSP-Greg MSP-Greg force-pushed the windows-actions branch 2 times, most recently from a558830 to 88d47a9 Compare January 30, 2020 23:26
@MSP-Greg MSP-Greg changed the title Windows Actions CI - warnings to 0 for bundle install Windows Actions CI - use $ErrorActionPreference for bundler install step Jan 30, 2020
@mojavelinux
Copy link
Member

Sure thing, take your time.

@MSP-Greg
Copy link
Contributor Author

MSP-Greg commented Jan 30, 2020

Thanks. I changed it to use $ErrorActionPreference. I'll write some code to locally see if I can determine what's going on. Don't know what you'd prefer...

@mojavelinux
Copy link
Member

But you didn't remove $env:RUBYOPT = '-W0', which we already determined fixes the issue. Did you mean to leave that in?

@MSP-Greg
Copy link
Contributor Author

Did you mean to leave that in?

I knew that. No, I did not. Sorry, fixed.

@mojavelinux
Copy link
Member

Yeah, that worked. Wow, now we have like 3 different ways of solving this issue.

I'll quickly summarize:

  1. Run $env:RUBYOPT = '-W0' before calling bundle install if bundle config is used in same step
  2. Run $ErrorActionPreference = 'Continue' before calling bundle install if bundle config is used in same step
  3. Move bundle config to separate step

Does that look right?

@MSP-Greg
Copy link
Contributor Author

Yes. I still can't repo this locally. Every command seems to exiting with zero. I hate this.

I think $ErrorActionPreference = 'Continue' is probably the best, as it shows what seems to be the problem...

@mojavelinux
Copy link
Member

Thanks again! I'll stick with the final result then. Cheers!

@mojavelinux mojavelinux merged commit 847765a into asciidoctor:master Jan 31, 2020
@MSP-Greg MSP-Greg deleted the windows-actions branch January 31, 2020 00:45
@slonopotamus
Copy link
Contributor

I'll stick with the final result then.

Splitting steps would keep build script cross-platform. You don't need that right now, though.

@mojavelinux
Copy link
Member

I went ahead and did both. That way we can monitor output in the interim, but disable the setting when it comes time to go cross platform.

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.

3 participants