Support symbolic links

[Elberet]

At this time, the FileSystem adapter explicitly ignores symbolic links when walking the source files:

https://github.com/SAP/ui5-fs/blob/f6803f512d47a6f305a0b0669f3c942350f40f2d/lib/adapters/FileSystem.js#L46

Is there a reason for this behaviour? My build pipeline involves custom tooling built around broccolijs and uses symlinks to avoid copying files unnecessarily between various build steps - this change in @ui5/fs breaks this for me.

Would you accept a PR that changes the hard-coded behaviour to allow symlinks? Or could this be configurable, and if so, how?

[matz3]

This seems to be related to https://github.com/SAP/ui5-fs/pull/140 (introduced with v1.1.0). According to the commit within the PR, it has been added to prevent loops in cyclic npm dependencies.

I'm not sure if this was just a "quick fix" or a more complex problem. Hopefully @RandomByte can shed some light here. Generally speaking your use case sounds reasonable to me, so I don't see a reason not allowing symlinks unless this causes some other major problems we can't solve otherwise.

Would you mind trying this with v1.0.2, which did not contain the followSymbolicLinks: false option?

[Elberet]

Yes, v1.0.2 processes my pipeline's output as expected.

Dealing with symlink loops should be an aspect of the glob library, but after some shallow digging it looks like globby / fast-glob make no efforts to detect cyclic links. Even a bandaid solution (exclude symlinks within the application root) isn't easily possible, since fast-glob doesn't expose @nodelib/fs.walk's entryFilter option. 🙁

[RandomByte]

This is interesting.

With our v1.1.0 we upgraded globby from 7.1.1 to 9.2.0. With these updates, globby switched from using node-glob to fast-glob.

Basically we tried to recreate the same behavior for our APIs as with globby 7.x. This meant mapping node-globs options (and their defaults) to the slightly different ones of fast-glob.

Apparently I can't remember where the cyclic loops mentioned in my commit actually occurred. But I think it was in a different context than with the PR. However, since node-glob documents its follow option as defaulting to false (note the text: All options are false by default, unless otherwise noted), I figured we need to set fast-globs followSymbolicLinks option to false as well to be compatible.

Now I'm wondering why v1.0.2 of @ui5/fs seems to follow symlinks in your case. globby documented its options as:

See the node-glob options in addition to the ones below. One difference is that nodir is true by default here.

And @ui5/fs did not explicitly set the follow option to true or false.

I think I'll have to do some validation of this behavior myself later.

@Elberet:

• Do you use symlinks or hardlinks?
• What OS are you on?
• What version of Node.js are you using?

[Elberet]

My development environment:

• Windows 10 1709.
• I'm using actual symbolic links, not NTFS junctions.
• Node.js is at v10.13.0.

My CI environment:

• Linux container based on the Docker node:10 image with headless Chromium.
• Still using symlinks - can't hardlink directories, after all.
• Node.js is at 10.16.0

The problem exists in both environments, so I'm fairly certain it's not platform related.

Instead, regarding the glob vs. fast-glob thing, I believe that glob's documentation is obfuscating the truth. If my reading of the code is correct, if follow is true, the library simply ignores whether an entry is a symlink or not and follows any and all entries forever; if follow is false, it instead records symlinks in a map and possibly prevents infinite loops from occurring - but it still traverses symlinked directories and matches symlinked files.

[RandomByte]

So I did some tests with https://github.com/SAP/ui5-fs/pull/163.

I have a test project which contains a nested application project. At some point I created a node_modules directory in that application project, which contains a cyclic symlink to a dependency of the surrounding project.

Now, when the surrounding project is being built, SAP/ui5-fs#163 throws with a "JavaScript heap out of memory" or "ENAMETOOLONG" error caused by the cyclic symlinks.

To reproduce:

# node v10.5.0 on macOS
git clone [email protected]:SAP/openui5.git
cd openui5
yarn
yarn link-all
cd src/sap.m/test/sap/m/demokit/cart
yarn
yarn link @openui5/sap.f @openui5/sap.m @openui5/sap.tnt @openui5/sap.ui.codeeditor @openui5/sap.ui.commons @openui5/sap.ui.core @openui5/sap.ui.demokit @openui5/sap.ui.documentation @openui5/sap.ui.dt @openui5/sap.ui.fl @openui5/sap.ui.integration @openui5/sap.ui.layout @openui5/sap.ui.rta @openui5/sap.ui.suite @openui5/sap.ui.support @openui5/sap.ui.table @openui5/sap.ui.unified @openui5/sap.ui.ux3 @openui5/sap.uxap @openui5/themelib_sap_fiori_3 @openui5/themelib_sap_belize @openui5/themelib_sap_bluecrystal
cd ../../../../../
ui5 build
# => "JavaScript heap out of memory" or "ENAMETOOLONG" error

Without SAP/ui5-fs#163 this error does not occur. Also, with @ui5/fs 1.0.2 (tested using @ui5/[email protected]) this error does not occur.

It's fair to say that this setup is arguable and surely not what we recommend. However, I had my OpenUI5 development repository like this for months now without issues 😅

I'm therefore currently leaning towards staying with the current behavior of not following symlinks by default. But I think we can add an option to follow symlinks for use cases like yours.

How do you consume the UI5 Tooling? Are you using the CLI or do you consume the @ui5/fs APIs directly in some custom Node.js scripts?

I'm just wondering whether you just need a lower level API or actually some ui5.yaml configuration.

[Elberet]

node-glob seems to be able to deal with nested, cyclic symlinks just fine... given this directory structure:

test
|-- a
|   `-- b
|       `-- a -> ../../a
|-- c
|   `-- x.js
`-- d -> ./c

and the glob pattern test/**/*.js, node-glob will produce the matches test/c/x.js and test/d/x.js - no errors or stack overflows.

Is the globby API really that much more preferable and/or fast-glob really that much faster compared to the old version that the regression in functionality is justified?

---

The extent to which I interact with the UI5 tooling basically boils down to this:

  async build() {
    const tree = await Ui5Project.normalizer.generateProjectTree({ cwd: this.inputPaths[0] })
    return Ui5Builder.builder.build({ tree, destPath: this.outputPath })
  }

At the time this code runs, the input directory contains a copy of the building project's package.json, an auto-generated minimal ui5.yml and the transpiled / compiled / browserified sources.

[RandomByte]

Is the globby API really that much more preferable and/or fast-glob really that much faster compared to the old version that the regression in functionality is justified?

fast-glob is indeed faster in our use case. We did some measurements back then between globby 7.x and 9.x:

Scenario: Building the OpenUI5 Sample App

Before: Average of 52,920 seconds over 10 runs
After: Average of 45,808 seconds over 10 runs
Difference: -7,112 seconds or -13,44%

Command: ui5 build --all
Benchmark tool: hyperfine with one warmup
Hardware: MacBook Pro 2017 - i7 2,9GHz/16GB

Scenario: Building the OpenUI5 Testsuite (i.e. all OpenUI5 libraries)

Before: Average of 212,279 seconds (≈3 minutes 32 seconds) over 10 runs
After: Average of 170,722 seconds (≈2 minutes 51 seconds) over 10 runs
Difference: -41,557 seconds or -19,58%

Command: ui5 build --all
Benchmark tool: hyperfine with one warmup
Hardware: MacBook Pro 2017 - i7 2,9GHz/16GB

---

After upgrading globby to 10.x, I repeated the OpenUI5 testsuite benchmarks and found another decrease of 21,40% compared to globby 9.x and 36,78% compared to globby 7.x.

So there is quite some motivation for us to stay with globby >10.

---

Regarding your project setup: Is it only your root project that contains symlinks or could it be possible that dependencies also contain symlinks that should be followed?

[Elberet]

Yeah, ~36% is considerable. I still feel that fast-glob is basically trading in strict correctness for speed, but oh well - this is probably not the place for a a discussion about programming philosophy. :wink:

---

Regarding symlinks in dependencies: while this is not yet the case, I do have plans to extend this build pipeline to generate UI5-style libraries as well as applications. When consumed within an application's build pipeline, the library's resources/ will probably be a symlink to some temporary directory.

[RandomByte]

Regarding symlinks in dependencies: while this is not yet the case, I do have plans to extend this build pipeline to generate UI5-style libraries as well as applications. When consumed within an application's build pipeline, the library's resources/ will probably be a symlink to some temporary directory.

So it's probably not a per-project configuration but rather something that depends on your build environment?

If that is the case, I could think of a CLI option --follow-symlinks (or similarly named). Possibly a settings section in your root projects ui5.yaml can default this flag to true for your setup so you don't need to always provide the CLI parameter.

What do you think?

[Elberet]

A CLI option is fine, but Ideally it should also be available as a flag for Ui5Project.normalizer.generateProjectTree.

[RandomByte]

Bummer, I forgot you're using the API directly. Of course it will be exposed on that level!

The normalizer does not make use of any globbing. I hope your project tree is still being generated just fine?

I would rather add a flag to the Ui5Builder.builder.build function. Which can pass it to the @ui5/fs resourceFactory.createCollectionsForTree which ultimately passes it to the FileSystem adapter.

So the CLI option is actually optional for a first implementation to solve your scenario.

We probably wont find time to work on implementing this for the next couple of weeks. If you would like to contribute what I outlined above, we'll be more than happy to review it!

[Elberet]

I've had an initial look and found that ui5-builder/lib/types/library/LibraryFormatter uses globby and, of course, passes followSymbolicLinks: false by default. What I can't find, however, are any invocations of the type's .format function.

I don't think it's safe to ignore these calls to globby, but I'd have to either add another positional parameter to the libraryType.format function or significantly change it's signature.

[RandomByte]

Hey @Elberet, just checking whether this is still a requirement for you?

[Elberet]

Well, my pipeline hasn't changed, but I've added a workaround step that makes a symlink-free copy of the project tree before handing that to the UI5 builder. It's arguably ugly and overhead, but it works and doesn't impact my daily development work.

I do still consider this at least a wart, but at this point it may as well be an issue of insufficient documentation than a bug.

[MichaelFlucher]

Would be nice if you include this option into the ui5.yaml. I can‘t use any pipeline and always have to manually copy the webapp folder to geht rid of the symlinks and adapt the ui5 configuration to use the new folder.