Skip to content

Fix tar extraction with windows-2016 image#62

Merged
eregon merged 1 commit into
ruby:masterfrom
MSP-Greg:win16-extract
Jun 10, 2020
Merged

Fix tar extraction with windows-2016 image#62
eregon merged 1 commit into
ruby:masterfrom
MSP-Greg:win16-extract

Conversation

@MSP-Greg
Copy link
Copy Markdown
Collaborator

@MSP-Greg MSP-Greg commented Jun 9, 2020

  1. common.js - add function win2nix(path) to convert windows style paths to nix style

  2. ruby_builder.js - use Git tar for windows-2016, leave windows-2019 as is

  3. Add three windows-2016 JRuby builds to test.yml

Closes #61

@MSP-Greg
Copy link
Copy Markdown
Collaborator Author

MSP-Greg commented Jun 9, 2020

We can add more tests, but the extraction was the main issue.

I left the logic the same as before for windows-2019, so when windows-2016 is removed, the logic using Git tar can be removed.

The mswin build does not work with windows-2016, as extension gems will not compile with VS 2017 and the build's libraries. I really don't see a need for generating another version.

@eregon
Copy link
Copy Markdown
Member

eregon commented Jun 10, 2020

Thanks for the fix!

Comment thread common.js Outdated
export function win2nix(path) {
return (/^[A-Z]:/i.test(path) ?
('/' + path[0].toLowerCase() + path.split(':')[1]) :
path).replace(/\\/g, '/').replace(/ /g, '\\ ')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you use a simple if and re-assign here?
I find a multi-line ternary condition hard to read.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done. Simplified, as js is pass by value, at least with 'primitive' types.

Comment thread .github/workflows/test.yml Outdated
run: which -a ruby rake
- name: where ruby, rake
if: matrix.os == 'windows'
if: startsWith(matrix.os, 'windows')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you leave this file unchanged, in order to avoid conflicts with #60 ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment thread ruby-builder.js Outdated
exec.exec(tar, [ '-xz', '-C', rubiesDir, '-f', downloadPath ]))
await common.measure('Extracting Ruby', async () => {
if (process.env.ImageOS === 'win16') {
await exec.exec('"C:\\Program Files\\Git\\usr\\bin\\tar.exe"', [ '-xz', '-C', common.win2nix(rubiesDir), '-f',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you extract the path to tar in a variable, similar to how it's done below?
Then the logic and arguments will nicely align with below.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

1. common.js - add function win2nix(path) to convert windows style paths to nix style

2. ruby_builder.js - use Git tar for windows-2016, leave windows-2019 as is
@eregon eregon merged commit 44338c5 into ruby:master Jun 10, 2020
@MSP-Greg MSP-Greg deleted the win16-extract branch June 10, 2020 22:23
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.

jruby fails on windows-2016

2 participants