Force `npm install`

[0x2b3bfa0]

@RCdeWit has found an error when using this iterative/setup-cml action in a self–hosted runner that, of course, already had cml installed.

@DavidGOrtega edits This pull request adds the -f option to “overwrite files recklessly [sic]” and workaround this issue.

• Checks if existing version coincides with desired version and if so do nothing
• Add force parameter to force the installation
• Logs the install output to be able to understand whats going on

The interesting/tricky thing is that uninstall then install does not always fail. However force is the only way of guarantee the complete installation. For the majority of the cases that this could happen (runner image and installed latest cml version) this code may suffice perfectly

[casperdcl]

• isn't the action meant to "uninstall the previous CML" first though?
• also windows tests failing

[DavidGOrtega]

isn't the action meant to "uninstall the previous CML" first though?

Exactly! The prior version should be removed first

[DavidGOrtega]

Changed p0 because seems to be needed for the release

[0x2b3bfa0]

isn't the action meant to "uninstall the previous CML" first though?

The previous CML is the binary we use to launch the runner. Not sure we actually want to uninstall that, or even overwrite it for that effect. 🤔

[0x2b3bfa0]

also windows tests failing

(Please kill me)

[DavidGOrtega]

The previous CML is the binary we use to launch the runner. Not sure we actually want to uninstall that, or even overwrite it for that effect. 🤔

😆 true!

But it works anyway

[0x2b3bfa0]

Yes, it works anyway or, at least, it should.

[0x2b3bfa0]

⚠️ On a second thought, overwriting the executable may crash the program

When you unlink(1) an executable while it's running, the file doesn't actually get deleted until the executable exits and the file descriptor reference count is zero. It has no effect on the running process.

When you overwrite an executable while it's running it may crash in very creative ways, because it's not loaded in memory at once; pages are loaded on demand.

[0x2b3bfa0]

@DavidGOrtega, TL;DR, it's fine to replace it when it's running, but we need to make sure that it's doing rm old && cp new old (delete & create) instead of cat new > old (overwrite)

[casperdcl]

would #57 be a better fix?

[0x2b3bfa0]

Yes, definitely, but we need to build Windows binaries, and https://github.com/iterative/setup-cml/pull/60#issuecomment-1097120247 still applies.

[DavidGOrtega]

@0x2b3bfa0 I knew! Yesterday I was trying to understand what is doing npm uninstall.

😉

- name: test cml
      run: |
        node --version
        cml --version
        # which cml
        # rm /usr/bin/cml
        npm uninstall -g @dvcorg/cml
        cml --version || echo 'no cml'

[DavidGOrtega]

When you overwrite an executable while it's running it may crash in very creative ways, because it's not loaded in memory at once; pages are loaded on demand.

My rationale is that in case of a node app its fine while we do not do it against node itself. Our code its just interpreted by the v8 engine, loaded in memory by the VM

[0x2b3bfa0]

My rationale is that in case of a node app its fine while we do not do it against node itself.

The pkg binaries are “node itself” plus our code, so yes, it's equally risky

Our code its just interpreted by the v8 engine, loaded in memory by the VM

Not sure... does V8 load all the code at once, or waits until it's needed to read the file? 🤔

[DavidGOrtega]

The pkg binaries are “node itself” plus our code, so yes, it's equally risky

I think I have not explained my self. Using the bin is more dangerous, the npm install its more secure because its the V8 VM loading our js

Not sure... does V8 load all the code at once, or waits until it's needed to read the file?

Everything unless we do the require inside non evaluated code. But we do the require in every single module on top. But I might be wrong

[0x2b3bfa0]

🤦🏼 Makes sense now. I thought you were talking about binaries.

[0x2b3bfa0]

Still, replacing binaries safely is straightforward (e.g. rm and cp as stated above) while installing a Node.js package might be a bit less stable. 🤔

[DavidGOrtega]

At current stage the workflow hangs forever in macos, something that does not happen installing the action.

jobs:
 cml:
    runs-on: [macos-latest]
    env:
      REPO_TOKEN: ${{ secrets.REPO_TOKEN }}
    steps:
      - uses: actions/checkout@v2
      - uses: iterative/setup-cml@v1

[0x2b3bfa0]

@DavidGOrtega, https://github.com/iterative/cml/pull/983 solves all your hanging issues in a rather drastic way. 🐉

[DavidGOrtega]

Unfortunately does not totally fix it for previous version of CML and we could end up with the same error but I have remembered this issue and I can pipe a dummy text as we do in formal CML tests.

I had to remove publish tests: In windows they fail. https://github.com/iterative/cml/pull/901 seems not to have fixed it?

[DavidGOrtega]

@0x2b3bfa0

[DavidGOrtega]

Also tests in container fails until https://github.com/iterative/cml/pull/974 is effective

[DavidGOrtega]

@iterative/cml ready to review 🙏

[0x2b3bfa0]

I had to remove publish tests: In windows they fail. https://github.com/iterative/cml/pull/901 seems not to have fixed it?

Windows tests are broken because an arcane issue involving the drive letter of the current working directory not being the same as the drive letter of the @npcz/magic installation path. 🎩 Magic.

• https://github.com/iterative/setup-cml/issues/56
• https://github.com/iterative/cml/pull/865

[0x2b3bfa0]

I can't approve “my own” pull request, by the way. 🙃

[0x2b3bfa0]

Attracting reviewers from @iterative/cml...

[DavidGOrtega]

@dacbd ready for review 🙏

[0x2b3bfa0]

@RCdeWit, this should be fixed now.