liferay / liferay-npm-tools

Collection of tools for using npm in Liferay
Other
18 stars 15 forks source link

Yarn Format duplicates the Copyright comment on Windows #207

Closed kresimir-coko closed 4 years ago

kresimir-coko commented 5 years ago

When running yarn format or yarn checkFormat on Windows the output is unexpected.

In the case of yarn format the formatter will append the 13 lines of Copyright comment under the already existing one.

In the case of yarn checkFormat the output is following:

$ yarn checkFormat
yarn run v1.15.2
warning package.json: No license field
$ liferay-npm-scripts check
src/main/resources/META-INF/resources/index.es.js: BAD
src/main/resources/META-INF/resources/liferay/aop/AOP.es.js: BAD
src/main/resources/META-INF/resources/liferay/compat/modal/Modal.es.js: BAD
src/main/resources/META-INF/resources/liferay/compat/progressbar/ProgressBar.es.js: BAD
src/main/resources/META-INF/resources/liferay/compat/slider/Slider.es.js: BAD
src/main/resources/META-INF/resources/liferay/compat/treeview/Treeview.es.js: BAD
src/main/resources/META-INF/resources/liferay/CompatibilityEventProxy.es.js: BAD
src/main/resources/META-INF/resources/liferay/component.es.js: BAD
src/main/resources/META-INF/resources/liferay/debounce/debounce.es.js: BAD
src/main/resources/META-INF/resources/liferay/DefaultEventHandler.es.js: BAD
src/main/resources/META-INF/resources/liferay/dom_task_runner.js: BAD
src/main/resources/META-INF/resources/liferay/DynamicInlineScroll.es.js: BAD
src/main/resources/META-INF/resources/liferay/events.js: BAD
src/main/resources/META-INF/resources/liferay/global.es.js: BAD
src/main/resources/META-INF/resources/liferay/ItemSelectorDialog.es.js: BAD
src/main/resources/META-INF/resources/liferay/keyboard-focus/KeyboardFocusManager.es.js: BAD
src/main/resources/META-INF/resources/liferay/layout_exporter.js: BAD
src/main/resources/META-INF/resources/liferay/lazy_load.js: BAD
src/main/resources/META-INF/resources/liferay/liferay.js: BAD
src/main/resources/META-INF/resources/liferay/modal/commands/OpenSimpleInputModal.es.js: BAD
src/main/resources/META-INF/resources/liferay/modal/components/SimpleInputModal.es.js: BAD
src/main/resources/META-INF/resources/liferay/portal.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/portlet.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/PortletInit.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/portlet_constants.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/portlet_util.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/register.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet/RenderState.es.js: BAD
src/main/resources/META-INF/resources/liferay/portlet.js: BAD
src/main/resources/META-INF/resources/liferay/PortletBase.es.js: BAD
src/main/resources/META-INF/resources/liferay/side_navigation.es.js: BAD
src/main/resources/META-INF/resources/liferay/toast/commands/OpenToast.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/address/get_countries.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/address/get_regions.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/fetch.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/form/get_form_element.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/form/object_to_form_data.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/form/post_form.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/form/set_form_values.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/format_storage.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/format_xml.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/get_crop_region.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/get_portlet_namespace.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/navigate.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/ns.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/object_to_url_search_params.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/portlet_url/create_action_url.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/portlet_url/create_portlet_url.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/portlet_url/create_render_url.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/portlet_url/create_resource_url.es.js: BAD
src/main/resources/META-INF/resources/liferay/util/to_char_code.es.js: BAD
src/main/resources/META-INF/resources/liferay/workflow.js: BAD
src/main/resources/META-INF/resources/loader/config.js: BAD
src/main/resources/META-INF/resources/misc/swfobject.js: BAD
src/main/resources/META-INF/resources/misc/xp_progress.js: BAD
test/liferay/aop/AOP.es.js: BAD
test/liferay/CompatibilityEventProxy.es.js: BAD
test/liferay/component.es.js: BAD
test/liferay/debounce/debounce.es.js: BAD
test/liferay/keyboard-focus/KeyboardFocusManager.es.js: BAD
test/liferay/portlet/action.es.js: BAD
test/liferay/portlet/create_resource_url.es.js: BAD
test/liferay/portlet/events.es.js: BAD
test/liferay/portlet/in_progress.es.js: BAD
test/liferay/portlet/mock/portlet_data.es.js: BAD
test/liferay/portlet/mock/setup.es.js: BAD
test/liferay/portlet/register.es.js: BAD
test/liferay/portlet/state.es.js: BAD
test/liferay/PortletBase.es.js: BAD
test/liferay/util/address/get_countries.es.js: BAD
test/liferay/util/address/get_regions.es.js: BAD
test/liferay/util/fetch.es.js: BAD
test/liferay/util/form/get_form_element.es.js: BAD
test/liferay/util/form/object_to_form_data.es.js: BAD
test/liferay/util/form/post_form.es.js: BAD
test/liferay/util/form/set_form_values.es.js: BAD
test/liferay/util/format_storage.es.js: BAD
test/liferay/util/format_xml.es.js: BAD
test/liferay/util/get_crop_region.es.js: BAD
test/liferay/util/get_portlet_namespace.es.js: BAD
test/liferay/util/mock/headers.es.js: BAD
test/liferay/util/mock/setup.es.js: BAD
test/liferay/util/navigate.es.js: BAD
test/liferay/util/ns.es.js: BAD
test/liferay/util/object_to_url_search_params.es.js: BAD
test/liferay/util/portlet_url/create_action_url.es.js: BAD
test/liferay/util/portlet_url/create_portlet_url.es.js: BAD
test/liferay/util/portlet_url/create_render_url.es.js: BAD
test/liferay/util/portlet_url/create_resource_url.es.js: BAD
test/liferay/util/to_char_code.es.js: BAD
Prettier checked 90 files, 90 files have problems
wincent commented 5 years ago

@kresimir-coko: can you post a gist of the actual changes it makes?

One thing I notice is that it is Prettier that is complaining about bad formatting (which makes me think something about line-endings, or the final trailing newline in the files), but the thing which inserts "missing" copyright headers is ESLint. I don't see any output from ESLint in what you pasted; is there more?

kresimir-coko commented 5 years ago

@wincent Here you go https://gist.github.com/kresimir-coko/73a5ec52f3499f7dcea5e45fcc1816e9

It's the same outcome for any *.js or *.es.js file

wincent commented 5 years ago

Thanks @kresimir-coko. Looking at the raw bytes I can only see 0x0a (ie. newlines), not 0x0d (ie. carriage returns), so that's not it. Will have to try and repro it on the Window lappy here.

What about my other question?

the thing which inserts "missing" copyright headers is ESLint. I don't see any output from ESLint in what you pasted; is there more?

wincent commented 5 years ago

Another doubt I have is whether you have any local configuration related to line endings that might be obscuring what's really happening (for example, the stuff in here).

kresimir-coko commented 5 years ago

Don't know why I omitted this part of the output when running yarn checkFormat. But here is the final few lines.

ESLint checked 79 files, found 0 errors, found 0 warnings
1 of 2 jobs failed
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
wincent commented 5 years ago

So that output doesn't report any changes, which means that's what you see once it has already added a duplicate header, right? (Because it doesn't then add a third, right?)

Looking at the hex dump of the file I can't see any difference between the two headers.

Output of first header (via xxd -c 30 create_portlet_url.es.js | head -n 20):

00000000: 2f2a 2a0a 202a 2043 6f70 7972 6967 6874 2028 6329 2032 3030 302d 7072 6573  /**. * Copyright (c) 2000-pres
0000001e: 656e 7420 4c69 6665 7261 792c 2049 6e63 2e20 416c 6c20 7269 6768 7473 2072  ent Liferay, Inc. All rights r
0000003c: 6573 6572 7665 642e 0a20 2a0a 202a 2054 6869 7320 6c69 6272 6172 7920 6973  eserved.. *. * This library is
0000005a: 2066 7265 6520 736f 6674 7761 7265 3b20 796f 7520 6361 6e20 7265 6469 7374   free software; you can redist
00000078: 7269 6275 7465 2069 7420 616e 642f 6f72 206d 6f64 6966 7920 6974 2075 6e64  ribute it and/or modify it und
00000096: 6572 0a20 2a20 7468 6520 7465 726d 7320 6f66 2074 6865 2047 4e55 204c 6573  er. * the terms of the GNU Les
000000b4: 7365 7220 4765 6e65 7261 6c20 5075 626c 6963 204c 6963 656e 7365 2061 7320  ser General Public License as 
000000d2: 7075 626c 6973 6865 6420 6279 2074 6865 2046 7265 650a 202a 2053 6f66 7477  published by the Free. * Softw
000000f0: 6172 6520 466f 756e 6461 7469 6f6e 3b20 6569 7468 6572 2076 6572 7369 6f6e  are Foundation; either version
0000010e: 2032 2e31 206f 6620 7468 6520 4c69 6365 6e73 652c 206f 7220 2861 7420 796f   2.1 of the License, or (at yo
0000012c: 7572 206f 7074 696f 6e29 0a20 2a20 616e 7920 6c61 7465 7220 7665 7273 696f  ur option). * any later versio
0000014a: 6e2e 0a20 2a0a 202a 2054 6869 7320 6c69 6272 6172 7920 6973 2064 6973 7472  n.. *. * This library is distr
00000168: 6962 7574 6564 2069 6e20 7468 6520 686f 7065 2074 6861 7420 6974 2077 696c  ibuted in the hope that it wil
00000186: 6c20 6265 2075 7365 6675 6c2c 2062 7574 2057 4954 484f 5554 0a20 2a20 414e  l be useful, but WITHOUT. * AN
000001a4: 5920 5741 5252 414e 5459 3b20 7769 7468 6f75 7420 6576 656e 2074 6865 2069  Y WARRANTY; without even the i
000001c2: 6d70 6c69 6564 2077 6172 7261 6e74 7920 6f66 204d 4552 4348 414e 5441 4249  mplied warranty of MERCHANTABI
000001e0: 4c49 5459 206f 7220 4649 544e 4553 530a 202a 2046 4f52 2041 2050 4152 5449  LITY or FITNESS. * FOR A PARTI
000001fe: 4355 4c41 5220 5055 5250 4f53 452e 2053 6565 2074 6865 2047 4e55 204c 6573  CULAR PURPOSE. See the GNU Les
0000021c: 7365 7220 4765 6e65 7261 6c20 5075 626c 6963 204c 6963 656e 7365 2066 6f72  ser General Public License for
0000023a: 206d 6f72 650a 202a 2064 6574 6169 6c73 2e0a 202a 2f0a 0a2f 2a2a 0a20 2a20   more. * details.. */../**. * 

Output of second header (via xxd -c 30 <(tail +594c create_portlet_url.es.js) | head -n 20):

00000000: 2f2a 2a0a 202a 2043 6f70 7972 6967 6874 2028 6329 2032 3030 302d 7072 6573  /**. * Copyright (c) 2000-pres
0000001e: 656e 7420 4c69 6665 7261 792c 2049 6e63 2e20 416c 6c20 7269 6768 7473 2072  ent Liferay, Inc. All rights r
0000003c: 6573 6572 7665 642e 0a20 2a0a 202a 2054 6869 7320 6c69 6272 6172 7920 6973  eserved.. *. * This library is
0000005a: 2066 7265 6520 736f 6674 7761 7265 3b20 796f 7520 6361 6e20 7265 6469 7374   free software; you can redist
00000078: 7269 6275 7465 2069 7420 616e 642f 6f72 206d 6f64 6966 7920 6974 2075 6e64  ribute it and/or modify it und
00000096: 6572 0a20 2a20 7468 6520 7465 726d 7320 6f66 2074 6865 2047 4e55 204c 6573  er. * the terms of the GNU Les
000000b4: 7365 7220 4765 6e65 7261 6c20 5075 626c 6963 204c 6963 656e 7365 2061 7320  ser General Public License as 
000000d2: 7075 626c 6973 6865 6420 6279 2074 6865 2046 7265 650a 202a 2053 6f66 7477  published by the Free. * Softw
000000f0: 6172 6520 466f 756e 6461 7469 6f6e 3b20 6569 7468 6572 2076 6572 7369 6f6e  are Foundation; either version
0000010e: 2032 2e31 206f 6620 7468 6520 4c69 6365 6e73 652c 206f 7220 2861 7420 796f   2.1 of the License, or (at yo
0000012c: 7572 206f 7074 696f 6e29 0a20 2a20 616e 7920 6c61 7465 7220 7665 7273 696f  ur option). * any later versio
0000014a: 6e2e 0a20 2a0a 202a 2054 6869 7320 6c69 6272 6172 7920 6973 2064 6973 7472  n.. *. * This library is distr
00000168: 6962 7574 6564 2069 6e20 7468 6520 686f 7065 2074 6861 7420 6974 2077 696c  ibuted in the hope that it wil
00000186: 6c20 6265 2075 7365 6675 6c2c 2062 7574 2057 4954 484f 5554 0a20 2a20 414e  l be useful, but WITHOUT. * AN
000001a4: 5920 5741 5252 414e 5459 3b20 7769 7468 6f75 7420 6576 656e 2074 6865 2069  Y WARRANTY; without even the i
000001c2: 6d70 6c69 6564 2077 6172 7261 6e74 7920 6f66 204d 4552 4348 414e 5441 4249  mplied warranty of MERCHANTABI
000001e0: 4c49 5459 206f 7220 4649 544e 4553 530a 202a 2046 4f52 2041 2050 4152 5449  LITY or FITNESS. * FOR A PARTI
000001fe: 4355 4c41 5220 5055 5250 4f53 452e 2053 6565 2074 6865 2047 4e55 204c 6573  CULAR PURPOSE. See the GNU Les
0000021c: 7365 7220 4765 6e65 7261 6c20 5075 626c 6963 204c 6963 656e 7365 2066 6f72  ser General Public License for
0000023a: 206d 6f72 650a 202a 2064 6574 6169 6c73 2e0a 202a 2f0a 0a69 6d70 6f72 7420   more. * details.. */..import 

Difference:

diff --git a/first b/second
index c62ed3f..651aaa0 100644
--- a/first
+++ b/second
@@ -17,4 +17,4 @@
 000001e0: 4c49 5459 206f 7220 4649 544e 4553 530a 202a 2046 4f52 2041 2050 4152 5449  LITY or FITNESS. * FOR A PARTI
 000001fe: 4355 4c41 5220 5055 5250 4f53 452e 2053 6565 2074 6865 2047 4e55 204c 6573  CULAR PURPOSE. See the GNU Les
 0000021c: 7365 7220 4765 6e65 7261 6c20 5075 626c 6963 204c 6963 656e 7365 2066 6f72  ser General Public License for
-0000023a: 206d 6f72 650a 202a 2064 6574 6169 6c73 2e0a 202a 2f0a 0a2f 2a2a 0a20 2a20   more. * details.. */../**. * 
+0000023a: 206d 6f72 650a 202a 2064 6574 6169 6c73 2e0a 202a 2f0a 0a69 6d70 6f72 7420   more. * details.. */..import 

As you can see, only the the differences are at the end (ie. where the import starts), which is exactly as expected. This is important because the linter is not supposed to add the header if it is not already there, but as you can see, it is there, byte-for-byte.

Probably not worth doing any more troubleshooting like this: I'll just have to repro it locally and dig from there.

kresimir-coko commented 5 years ago

Good luck! šŸ—” šŸ—” šŸ—”

wincent commented 5 years ago

Is this still happening for you @kresimir-coko?

I tried reproducing this on the Windows laptop here to no avail. Doesn't change any files when run from the top-level "modules/" folder, or from a subproject like "frontend-js-web".

wincent commented 5 years ago

It's possible that you might have some transparent line-ending conversion going on in Git whenever you checkout. Which in turn means that (something like) this commit just might fix it (on an unrelated PR). You might be able to fix this locally with a git config core.eol lf (or something like that). Just in case that one gets force-pushed and goes away, this is it:

From 5ca2873a6d9dadf8f66cd4e0b359a058a17d2515 Mon Sep 17 00:00:00 2001
From: Greg Hurrell <greg@hurrell.net>
Date: Fri, 13 Sep 2019 22:58:01 +0200
Subject: [PATCH] fix: add a .gitattributes file to unbreak Travis on Windows

My suspicion is that when Travis does its Git checkout, it converts all
LF to CRLF, and that breaks the snapshots because all the indices
change; example:

https://travis-ci.org/liferay/liferay-npm-tools/jobs/584751075

See:

- https://help.github.com/en/articles/configuring-git-to-handle-line-endings
- https://git-scm.com/docs/gitattributes
---
 .gitattributes | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 .gitattributes

diff --git a/.gitattributes b/.gitattributes
new file mode 100644
index 0000000..b4195e7
--- /dev/null
+++ b/.gitattributes
@@ -0,0 +1,6 @@
+# Don't let Git transform line-endings to CRLF on Windows, because that
+# would break the snapshots.
+*.js text eol=lf
+*.jsp text eol=lf
+*.jspf text eol=lf
+*.js.snap text eol=lf
-- 
2.20.1
jbalsas commented 5 years ago

I was going to say this was not possible because we already had a .gitattributes in place at liferay-portal/.gitattributes but it turns out it's only for pom and ivy.xml files.

We used to apply it for soy files when the Soy Compiler had trouble with line endings in windows, but that got fixed by the compiler itself, so maybe this is also something that can be taken care of somewhere else?

wincent commented 5 years ago

There are two "somewhere elses" apart from an actual .gitattributes file that I can think of: one is inside eslint-plugin-notice (which we don't control, although we might be able to submit a PR) and the other is via git config like I said above (which isn't ideal because every Windows developer would have to do it).

I guess the other thing we could do is something hacky like dynamically detect when running on windows and pass an alternate templateFile path to eslint-plugin-notice; we could construct that template file on the fly by converting the line-endings and writing out a new file to a temporary location.

Still, I'm curious to know why I couldn't repro on the Windows laptop. Maybe because I was using the fancy shell that gets installed with Git.

wincent commented 4 years ago

Given that we don't have a reliable repro, I am going to close this as non-actionable. If somebody can provide repro steps that work, we'll reopen.