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

deno upgrade command #4328

Merged
merged 25 commits into from
Mar 23, 2020
Merged

deno upgrade command #4328

merged 25 commits into from
Mar 23, 2020

Conversation

bartossh
Copy link
Contributor

  • Link to issue

  • Used new library to unzip archive on windows / macos / unix systems: flate2 link

  • If any of information printed to the user should be changed, will gladly do. I haven't been sure what is best way to communicate actions taken while upgrade happens.

  • There is no progress indicator of download that happens in the background, and at this point I have decided to not implement this feature. If there is some, please point me where it is and I would gladly use it it.

@bartossh bartossh changed the title Bartossh/deno upgrade deno upgrade command Mar 16, 2020
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

@bartossh Oh wow, I totally missed this PR. Thanks! I'm hesitant on the flate2 dependency, I'm going to try to poke at this code and see if I can remove it.

Copy link
Contributor Author

@bartossh bartossh left a comment

Choose a reason for hiding this comment

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

@ry I can think of how to remove the flate2 crate if you like. Will try to find some alternative solution in this week. Is it ok?
@kevinkassimo I will apply all your suggestions after will have response what next from @ry, to make all fixes in one go, as there I assume, may be much more of issues to fix. Thx Guys.

cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
@ry
Copy link
Member

ry commented Mar 19, 2020

@bartossh How will you remove it?

I was thinking about taking inspiration from how we do it in deno_install

https://github.com/denoland/deno_install/blob/master/install.sh

https://github.com/denoland/deno_install/blob/master/install.ps1

cli/flags.rs Outdated Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
@bartossh
Copy link
Contributor Author

bartossh commented Mar 21, 2020

@ry I can remove flate2 totaly and call from Rust system programs to unzip downloaded archive, like so:

if ($IsWindows) {
  Expand-Archive $DenoZip -Destination $BinDir -Force
  Remove-Item $DenoZip
} else {
  gunzip -df $DenoZip
}

And then replace the existing binary and chmod +x new one for unix like os.

@bartossh bartossh requested a review from ry March 21, 2020 16:45
@ry
Copy link
Member

ry commented Mar 21, 2020

@bartossh I've made some changes to remove the dependency on flate2. I wonder if you could replace my todo!() to get it working on Windows?

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

still needs to work on windows

@bartossh
Copy link
Contributor Author

bartossh commented Mar 22, 2020

Sure, I will do. 👍

@bartossh
Copy link
Contributor Author

FYI.

While making upgrade to unzip on Windows (I haven't yet tested it properly), I encountered an issue while renaming files place in the code

This is the backtrace.

  • short:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 18, kind: Other, message: "Invalid cross-device link" }', cli/upgrade.rs:175:3
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  • long:
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 18, kind: Other, message: "Invalid cross-device link" }', cli/upgrade.rs:175:3
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write
             at src/libcore/fmt/mod.rs:1052
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1426
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:204
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:224
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:472
  11: rust_begin_unwind
             at src/libstd/panicking.rs:380
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  13: core::option::expect_none_failed
             at src/libcore/option.rs:1199
  14: core::result::Result<T,E>::unwrap
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libcore/result.rs:963
  15: deno::upgrade::replace_exe
             at cli/upgrade.rs:175
  16: deno::upgrade::upgrade_command::{{closure}}
             at cli/upgrade.rs:99
  17: <std::future::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/future.rs:43
  18: <core::pin::Pin<P> as core::future::future::Future>::poll
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libcore/future/future.rs:119
  19: tokio::runtime::basic_scheduler::BasicScheduler<P>::block_on
             at /home/bartossh/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.13/src/runtime/basic_scheduler.rs:138
  20: tokio::runtime::Runtime::block_on::{{closure}}
             at /home/bartossh/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.13/src/runtime/mod.rs:413
  21: tokio::runtime::context::enter
             at /home/bartossh/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.13/src/runtime/context.rs:72
  22: tokio::runtime::handle::Handle::enter
             at /home/bartossh/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.13/src/runtime/handle.rs:34
  23: tokio::runtime::Runtime::block_on
             at /home/bartossh/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-0.2.13/src/runtime/mod.rs:410
  24: deno::tokio_util::run_basic
             at cli/tokio_util.rs:18
  25: deno::main
             at cli/lib.rs:500
  26: deno::main
             at cli/main.rs:4
  27: std::rt::lang_start::{{closure}}
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/rt.rs:67
  28: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  29: std::panicking::try::do_call
             at src/libstd/panicking.rs:305
  30: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:86
  31: std::panicking::try
             at src/libstd/panicking.rs:281
  32: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  33: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  34: std::rt::lang_start
             at /rustc/b8cedc00407a4c56a3bda1ed605c6fc166655447/src/libstd/rt.rs:67
  35: main
  36: __libc_start_main
  37: _start
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

I am wondering if this is only going on, on my OS or maybe it is a wider problem.
My OS: 5.4.24-1-MANJARO #1 SMP PREEMPT Thu Mar 5 20:29:25 UTC 2020 x86_64 GNU/Linux

I will try to test it on fresh Ubuntu asap.

cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
cli/upgrade.rs Outdated Show resolved Hide resolved
@bartossh
Copy link
Contributor Author

Ok I see @ry fixed it for Windows and for Arch based Linux it works with this change: fs::rename(new, old).or_else(|_| fs::copy(new, old).map(|_| ()))?;. rename wasn't working for same reason as Windows.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thank you very much @bartossh !

@ry ry merged commit ec07386 into denoland:master Mar 23, 2020
@bartossh bartossh deleted the bartossh/deno_upgrade branch March 23, 2020 15:40
@bartlomieju bartlomieju mentioned this pull request Apr 1, 2020
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.

4 participants