Skip to content

Conversation

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Sep 2, 2020

Fixes part of #52 - doesn't work with gems.rb.

@ioquatix ioquatix requested a review from eregon September 2, 2020 22:24
@ioquatix ioquatix merged commit 638e8ff into ruby:master Sep 2, 2020
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

It looks like you missed this case:

const gemfilePath = `${process.env['BUNDLE_GEMFILE'] || 'Gemfile'}.lock`

@eregon
Copy link
Member

eregon commented Sep 3, 2020

Thanks for the PR.
Please let me review before merging since I'm the maintainer.

@eregon
Copy link
Member

eregon commented Sep 3, 2020

Is there any reason to use gems.rb over Gemfile?
Any adoption of that? It seems extremely few projects use it.

I'm not looking forward to support every possible experimental Bundler feature, due to the cost to maintain that.
It seems Rails also considers it as a rather pointless change: rails/rails#31295

if (!fs.existsSync('Gemfile')) {
console.log('No Gemfile, skipping "bundle install" and caching')
return
if (await bundleInstallSpecific('gems.rb', 'gems.locked', platform, engine, version)) {
Copy link
Member

Choose a reason for hiding this comment

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

bundleInstallSpecific does not return in the successful case.
Does the if work here? It doesn't seem so:

> async function foo() { console.log("foo"); }
> async function call() { var r=await foo(); console.log(r); if (r) { console.log("OK") } else { console.log("NOPE") } }
undefined
> call()
foo
Promise { <pending> }
> undefined
NOPE

@eregon
Copy link
Member

eregon commented Sep 3, 2020

I'll revert this, let's review & discuss it properly before merging.

@ioquatix
Copy link
Member Author

ioquatix commented Sep 3, 2020

I’m already using it so please don’t revert it. Why don’t we just fix the issues you have. I tested both cases in my own repos and it seemed to work fine.

@eregon
Copy link
Member

eregon commented Sep 3, 2020

I’m already using it

How, since it's not released?
Please make a new PR and let's get it fully ready before merge & release.

@ioquatix
Copy link
Member Author

ioquatix commented Sep 3, 2020

@eregon
Copy link
Member

eregon commented Sep 3, 2020

I see, sorry about that.
Using @master is expected to be unstable: https://github.com/actions/toolkit/blob/master/docs/action-versioning.md#compatibility

@ioquatix
Copy link
Member Author

ioquatix commented Sep 3, 2020

Sure, I completely understand your POV, but I think it warranted just a tiny bit more discussion, given how easy it is to make the changes you requested.

@ioquatix
Copy link
Member Author

ioquatix commented Sep 3, 2020

Can you please review #84

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.

2 participants