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

Rebuild modulePath correctly in grunt _build:notice if on Windows #11439

Merged
merged 1 commit into from
May 10, 2017

Conversation

colmose
Copy link
Contributor

@colmose colmose commented Apr 26, 2017

The module paths are split by ':' and with a Windows filepath (and the 'C:/' prefix), the split returns the drive letter and directory path separately. This causes the modulePath to be set incorrectly into the packagePaths object and the subsequent call for the licenses by key returns undefined on Windows only.

This branch recombines the drive and directory paths and sets the correct key into packagePaths.

Fixes #11408

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

@jbudz jbudz self-assigned this Apr 26, 2017
@jbudz
Copy link
Member

jbudz commented Apr 26, 2017

jenkins, test it

@tbragin tbragin added the Team:Operations Team label for Operations Team label Apr 26, 2017
@epixa epixa changed the title Rebuild modulePath correctly in grunt _build:notice if on Windows-#11408 Rebuild modulePath correctly in grunt _build:notice if on Windows Apr 26, 2017
@epixa epixa added the review label Apr 26, 2017
@jbudz
Copy link
Member

jbudz commented Apr 26, 2017

@LeeDr if you get a chance would you mind giving this a look too?

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

This LGTM. There's one other issue I noticed here, we need to check for both forward and back slashes /.*(\/|\\)kibana(\/|\\)/. We can cover it in a followup PR.

@colmose
Copy link
Contributor Author

colmose commented May 9, 2017

I can update the PR this evening with that regex, if you fancy.

Only take two mins and sits in this PR nicely.

@jbudz
Copy link
Member

jbudz commented May 9, 2017

👍 works for me

@colmose
Copy link
Contributor Author

colmose commented May 9, 2017

Alternation not alteration. 😠

@LeeDr LeeDr self-requested a review May 9, 2017 19:58
Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM

I got past the _build:notice part on my Windows 10.

I'll file a new issue on this failure (I don't think it could be caused by this PR. I don't think I ever got this far on Windows before);

Running "_build:archives" task
 > tar -zchf C:\git\kibana\target\kibana-6.0.0-alpha1-SNAPSHOT-darwin-x86_64.tar.gz kibana-6.0.0-alpha1-SNAPSHOT-darwin-x86_64
 > tar -zchf C:\git\kibana\target\kibana-6.0.0-alpha1-SNAPSHOT-linux-x86_64.tar.gz kibana-6.0.0-alpha1-SNAPSHOT-linux-x86_64
 > tar -zchf C:\git\kibana\target\kibana-6.0.0-alpha1-SNAPSHOT-linux-x86.tar.gz kibana-6.0.0-alpha1-SNAPSHOT-linux-x86
 > zip -rq -ll C:\git\kibana\target\kibana-6.0.0-alpha1-SNAPSHOT-windows-x86.zip kibana-6.0.0-alpha1-SNAPSHOT-windows-x86
Warning: Command failed: tar -zchf C:\git\kibana\target\kibana-6.0.0-alpha1-SNAPSHOT-darwin-x86_64.tar.gz kibana-6.0.0-alpha1-SNAPSHOT-darwin-x86_64
tar (child): Cannot connect to C: resolve failed
 Use --force to continue.

Aborted due to warnings.

@jbudz
Copy link
Member

jbudz commented May 9, 2017

@colmose thanks for the updates. If you get a chance can you rebase or merge master in? CI is failing on a sub node_module issue that we've fixed in master since.

The module paths are split by ':' and with a Windows filepath (and the 'C:/' prefix),
the split returns the drive letter and directory path separately.
This causes the modulePath to be set incorrectly into the packagePaths object
and the subsequent call for the licenses by key returns undefined on Windows only.

This commit recombines the drive and directory paths and sets the correct key into packagePaths.
@colmose
Copy link
Contributor Author

colmose commented May 9, 2017

Done, was just waiting for a thumbs up before tidying up after myself! 👍

@colmose
Copy link
Contributor Author

colmose commented May 9, 2017

@LeeDr, I'm not on a windows box for a few days, can you test this branch of my fork?

That might sort the tar issue (based on this)

@jbudz
Copy link
Member

jbudz commented May 9, 2017

jenkins, test it

@jbudz
Copy link
Member

jbudz commented May 10, 2017

LGTM. Thank you for the PR and quick responses @colmose.

@jbudz jbudz merged commit 68ada6b into elastic:master May 10, 2017
jbudz pushed a commit to jbudz/kibana that referenced this pull request May 10, 2017
The module paths are split by ':' and with a Windows filepath (and the 'C:/' prefix),
the split returns the drive letter and directory path separately.
This causes the modulePath to be set incorrectly into the packagePaths object
and the subsequent call for the licenses by key returns undefined on Windows only.

This commit recombines the drive and directory paths and sets the correct key into packagePaths.
@colmose
Copy link
Contributor Author

colmose commented May 10, 2017

No problem, I'll have a look at that tar issue when I get onto a windows box.

jbudz added a commit that referenced this pull request May 10, 2017
The module paths are split by ':' and with a Windows filepath (and the 'C:/' prefix),
the split returns the drive letter and directory path separately.
This causes the modulePath to be set incorrectly into the packagePaths object
and the subsequent call for the licenses by key returns undefined on Windows only.

This commit recombines the drive and directory paths and sets the correct key into packagePaths.
snide pushed a commit to snide/kibana that referenced this pull request May 30, 2017
The module paths are split by ':' and with a Windows filepath (and the 'C:/' prefix),
the split returns the drive letter and directory path separately.
This causes the modulePath to be set incorrectly into the packagePaths object
and the subsequent call for the licenses by key returns undefined on Windows only.

This commit recombines the drive and directory paths and sets the correct key into packagePaths.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Operations Team label for Operations Team v5.5.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm run build -- --skip-os-packages fails on Windows "Cannot read property 'licenses' of undefined"
6 participants