fixes #180 : Enable syntax highlight to all platform #352

Geobert · 2018-01-02T16:16:36Z

No description provided.

epage · 2018-01-02T16:18:53Z

Cargo.toml

 optional = true
-default-features = false
+default-features = true


Is there a reason you are enabling the default features of syntect?

I admit it was kind of trials and errors here ^^' Not sure this one is needed.

epage · 2018-01-02T16:19:54Z

Cargo.toml

-dev = []
+default = ["syntax-highlight"]
+unstable = ["syntax-highlight"]
+dev = ["syntax-highlight"]


Why specify it in dev and unstable? default should be sufficient

I didn't test with only default, I test now :)

epage · 2018-01-02T16:20:50Z

Cargo.toml

-default = []
-unstable = []
-dev = []
+default = ["syntax-highlight"]


The CI should do a test build with default features disabled.

Also, the CI should no longer explicitly call out syntax-highlight if its enabled by default.

Oh, I forgot to edit the CI scripts, going right now :)

Oh, missed the deploy script. Thanks for getting that. So just .travis.yml and appveyor.yml left?

appveyor has no mention of the feature, I've changed travis :)

appveyor has no mention of the feature, I've changed travis :)

My concern is with us accidentally breaking the no-default build.

The CI should do a test build with default features disabled.

hm, I don't know much about appveyor yet, what do you mean?

This is a Cargo feature and not an appveyor vs travis thing.

syntax-highlight is a default feature, meaning cargo will auto-add it to the build when calling cargo build. Users can disable this though, with --no-default-features. We should make sure we don't break the build when no features are enabled.

So its just a matter of adding to both CIs
cargo test --no-default-features.

I see! :D I will add this after fixing appveyor :)

epage · 2018-01-02T16:21:02Z

Thanks for owning this!

Geobert · 2018-01-02T17:03:14Z

Shall we keep the gnu target in appveyor?

epage · 2018-01-02T17:09:32Z

appveyor.yml

@@ -22,6 +22,7 @@ environment:
 install:
  - ps: >-
      $Env:PATH += ';C:\msys64\usr\bin'
+  - if "%TARGET%" == "x86_64-pc-windows-msvc" call "%VS140COMNTOOLS%\..\..\VC\vcvarsall.bat" x86_amd64


You'll probably need to copy the rest of the setup for this from gutenberg
https://github.com/Keats/gutenberg/blob/master/appveyor.yml

yeah, I'm trying to figure out what's wrong ^^ bear with me, it will be a git mess in the end x)

Geobert · 2018-01-02T21:44:08Z

I don't get why appveyor is failing, cargo test runs perfectly fine on my computer :(

epage · 2018-01-02T22:00:37Z

It seems like you made it the furthest when you had --target %TARGET passed to cargo.

Geobert · 2018-01-02T22:03:06Z

with --target %TARGET it does not work on my computer :-/ I'm trying the os line which is present in gutenberg

epage · 2018-01-02T22:09:42Z

with --target %TARGET it does not work on my computer :-/

How come --target x86_64-pc-windows-msvc fail on your computer?

Geobert · 2018-01-02T22:19:56Z

Hm, it did failed but I think I forgot the call to the batch file… ^^' it works so I put back the --target

Geobert · 2018-01-02T22:35:25Z

the cli tests are all failing, don't know why :(

epage · 2018-01-02T22:50:42Z

My guess is that its something to do with the tempdir.

I'd recommend

Disabling travis within your branch (to be nicer to travis while you iterate)
Moving down to one appveyor target for now (to be nicer to appveyor while you iterate)
Try setting RUST_BACKTRACE to get more info.

epage · 2018-01-02T22:52:14Z

Shall we keep the gnu target in appveyor?

Forgot to respond to this

This goes back to #99 for which I have no clue how important that target is within the rust ecosystem. Is there a reason you disabled it besides faster iteration?

Geobert · 2018-01-02T22:59:36Z

how do we disable travis?

epage · 2018-01-02T23:07:19Z

One option is to rename the file.

Geobert · 2018-01-03T11:09:15Z

Ok I found what's going on: passing --target change the path of cobalt.exe from target/debug/cobalt to target/x86_64-pc-windows-msvc/debug/cobalt

I'm going to add an instruction in appveyor.yml to copy the exec file just under target and change the cli test to launch target/cobalt instead

Geobert · 2018-01-03T14:37:10Z

Travis failed here because of https://travis-ci.org/cobalt-org/cobalt.rs/jobs/324590284#L578 (error while downloading rust)

It should work. How do we force a rebuild?

epage · 2018-01-03T16:14:34Z

Ok I found what's going on: passing --target change the path of cobalt.exe from target/debug/cobalt to target/x86_64-pc-windows-msvc/debug/cobalt

Alternatively, we could use main_binary as long as we don't need extra features enabled (which these tests shouldn't need)

https://docs.rs/assert_cli/0.5.4/assert_cli/struct.Assert.html#method.main_binary

epage · 2018-01-03T16:15:45Z

appveyor.yml

@@ -9,29 +11,39 @@ environment:

  matrix:
    # Stable channel
-    - TARGET: x86_64-pc-windows-gnu
+    #- TARGET: x86_64-pc-windows-gnu


Whats the status of re-enabling this?

Is does not compile, failing on miniz

epage · 2018-01-03T16:17:34Z

Things remaining

Consider using main_binary
window-gnu?
running tests with default features disabled

And then once all looks good on that, cleaning up commits.

Geobert · 2018-01-03T16:34:36Z

What is main_binary?
I'm for abandoning gnu target, what's the point if we support Windows through MSVC?
oh yeah forgot about this, I'll tackle this tomorrow :)

epage · 2018-01-03T16:54:49Z

What is main_binary?

main_binary is feature in assert_cli that will find the binary for you.

I'm for abandoning gnu target, what's the point if we support Windows through MSVC?

grumble Ok.

Geobert · 2018-01-03T18:30:55Z

replacing all command by main_binary and moving the parameters into with_args(&[…]) makes two tests to fail, only 2 >_< :

failures:

---- init_project_can_build stdout ----
        thread 'init_project_can_build' panicked at 'CLI assertion failed: (command `cargo run --bin cobalt -- init --trace` expected to succeed)
status=failed
stdout=``````
stderr=```error: could not find `Cargo.toml` in `C:\Users\Geob\AppData\Local\Temp\init.T2gAKVXg42pr` or any parent directory
```', C:\Users\Geob\.cargo\registry\src\github.com-1ecc6299db9ec823\assert_cli-0.5.4\src\assert.rs:368:12
stack backtrace:

---- log_levels_silent stdout ----
        thread 'log_levels_silent' panicked at 'CLI assertion failed: `cargo run -- build --silent -d C:\Users\Geob\AppData\Local\Temp\silent.AVccq2wIlRDv` StdErr mismatch: diff:

+Blocking waiting for file lock on build directory
    Finished dev [unoptimized + debuginfo] target(s) in 0.1 secs
     Running `I:\Dev\cobalt.rs\target\debug\cobalt.exe build --silent -d C:\Users\Geob\AppData\Local\Temp\silent.AVccq2wIlRDv`
', C:\Users\Geob\.cargo\registry\src\github.com-1ecc6299db9ec823\assert_cli-0.5.4\src\assert.rs:368:12
stack backtrace:
   0:     0x7ff6aefe95b1 - std::sys_common::backtrace::_print
                               at C:\projects\rust\src\libstd\sys_common\backtrace.rs:92
   1:     0x7ff6aeff5b2c - std::panicking::default_hook::{{closure}}
                               at C:\projects\rust\src\libstd\panicking.rs:380
   2:     0x7ff6aeff57a7 - std::panicking::default_hook
                               at C:\projects\rust\src\libstd\panicking.rs:391
   3:     0x7ff6aeff6195 - std::panicking::rust_panic_with_hook
                               at C:\projects\rust\src\libstd\panicking.rs:577
   4:     0x7ff6aeff6028 - std::panicking::begin_panic<alloc::string::String>
                               at C:\projects\rust\src\libstd\panicking.rs:538
   5:     0x7ff6aeff5f01 - std::panicking::begin_panic_fmt
                               at C:\projects\rust\src\libstd\panicking.rs:522
   6:     0x7ff6aef2db43 - assert_cli::assert::Assert::unwrap
                               at C:\Users\Geob\.cargo\registry\src\github.com-1ecc6299db9ec823\assert_cli-0.5.4\src\assert.rs:368
   7:     0x7ff6aef22f7a - cli::log_levels_silent
                               at I:\Dev\cobalt.rs\tests\cli.rs:138
   8:     0x7ff6aefa943d - test::{{impl}}::call_box<(),closure>
                               at C:\projects\rust\src\libtest\lib.rs:141
   9:     0x7ff6aeff74d1 - panic_unwind::__rust_maybe_catch_panic
                               at C:\projects\rust\src\libpanic_unwind\lib.rs:99
  10:     0x7ff6aef97342 - std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()>
                               at C:\projects\rust\src\libstd\sys_common\backtrace.rs:134
  11:     0x7ff6aef98687 - std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>
                               at C:\projects\rust\src\libstd\panicking.rs:476
  12:     0x7ff6aeff74d1 - panic_unwind::__rust_maybe_catch_panic
                               at C:\projects\rust\src\libpanic_unwind\lib.rs:99
  13:     0x7ff6aefa22d2 - alloc::boxed::{{impl}}::call_box<(),closure>
                               at C:\projects\rust\src\liballoc\boxed.rs:726
  14:     0x7ff6aeff4a5b - std::sys::imp::thread::{{impl}}::new::thread_start
                               at C:\projects\rust\src\libstd\sys\windows\thread.rs:54
  15:     0x7ffd978c8363 - BaseThreadInitThunk


failures:
    init_project_can_build
    log_levels_silent

test result: FAILED. 6 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test cli'

I don't get it, it should do the exact same thing.

Geobert · 2018-01-03T18:36:51Z

here the gist: https://gist.github.com/Geobert/a989ea06281b0fad39ee2bc67269ad06

epage · 2018-01-03T19:14:14Z

Huh, didn't realize cargo run will print out that text with --quiet.

See more info at the issue I created on assert_cli: assert-rs/assert_cli#79

epage · 2018-01-03T19:22:28Z

Ok, so that assert_cli issue will probably not be resolved in time to unblock this issue.

The main care abouts: people can easily run the tests on their own (no extra steps) and the CI provides value.

main_binary fails the former because it requires knowing that you need to pass a flag to limit the number of threads

The original solution fails because it requires copying the binary first, even outside of the --target case (see .travis.yml).

Another idea I have is that instead of --target %TARGET%, would rustup run %CHANNEL%-%TARGET% cargo ... work and not modify the path the binary is written to?

epage · 2018-01-03T19:23:23Z

Another idea I have is that instead of --target %TARGET%, would rustup run %CHANNEL%-%TARGET% cargo ... work and not modify the path the binary is written to?

I guess the shorthand would also work: cargo +%CHANNEL%-%TARGET% ...

epage · 2018-01-04T14:07:40Z

appveyor.yml

@@ -1,6 +1,8 @@
 # Based on the "trust" template v0.1.1
 # https://github.com/japaric/trust/tree/v0.1.1

+os: Visual Studio 2015
+
 environment:
  global:
    RUST_VERSION: stable


This is dead code now, please remove it

Geobert · 2018-01-05T11:39:13Z

:o everything passes! need to clean up commits now

Geobert · 2018-01-05T12:09:23Z

SIGKILL on a compilation? https://travis-ci.org/cobalt-org/cobalt.rs/jobs/325410957#L796

I've put back check for travis, I think SIGKILL is a timeout isssue

epage

Looks like the last of it!

epage · 2018-01-06T16:05:53Z

appveyor.yml

-    - TARGET: i686-pc-windows-msvc
-      CHANNEL: stable
+    #- TARGET: x86_64-pc-windows-gnu
+    #  CHANNEL: stable


If we're not supporting gnu, we should remove it (Rather than leave it commented out) and give a reason in the commit message.

epage · 2018-01-06T16:06:06Z

appveyor.yml

+    #- TARGET: x86_64-pc-windows-msvc
+    #  CHANNEL: beta
+    #  TOOLCHAIN: msvc
+    #  PLATFORM: x86_64


These targets are still commented out.

Geobert · 2018-01-06T17:13:28Z

I don't get this error: https://ci.appveyor.com/project/johannhof/cobalt-rs/build/1.0.571/job/ptn77qeecg26q01i#L972

this happens time to time without apparent reason :-/

epage · 2018-01-07T02:08:12Z

Whats weird is this doesn't fail as often on other builds.

Geobert · 2018-01-07T10:22:24Z

https://ask.electric-cloud.com/questions/6390/link-fatal-error-lnk1318-unexpected-pdb-error.html

Geobert · 2018-01-07T10:52:53Z

and now it on x86_64 instead on i686… (╯°□°）╯︵ ┻━┻

Geobert · 2018-01-07T10:56:36Z

rust-lang/cargo#4655
rust-lang/cargo#3161

seems that we can't do anything about it? Maybe adding syntax-highlight makes the build consume more RAM?

epage · 2018-01-08T16:04:29Z

appveyor.yml

@@ -29,6 +29,7 @@ install:
  - if "%TOOLCHAIN%" == "msvc" if "%PLATFORM%" == "x86_64" "C:\Program Files\Microsoft SDKs\Windows\v7.1\Bin\SetEnv.cmd" /x64
  - if "%TOOLCHAIN%" == "msvc" if "%PLATFORM%" == "x86_64" call "%VS140COMNTOOLS%\..\..\VC\vcvarsall.bat" x86_amd64
  - if "%TOOLCHAIN%" == "msys" set PATH=C:\msys64\mingw%MSYS_BITS%\bin;C:\msys64\usr\bin;%PATH%
+  - set MSBUILD_NODE_CONNECTION_TIMEOUT=0


Should this instead be under environment: global:?

epage · 2018-01-08T16:06:01Z

Now I'm trying to weigh out dropping 32-bit support.

The main use case of keeping it afaik is MS's new x86 on ARM stuff which is 32-bit only. Unsure if we'd be able to cross compile to it at some point.

Geobert · 2018-01-09T17:00:49Z

I agree to get rid of 32bits build, I'll try that asap, holidays are finished…

epage · 2018-01-09T17:03:14Z

i686-pc-windows-msvc builds are already broken for release builds due to issues with GetLastError?

One build back with 1.22.1, see https://ci.appveyor.com/project/johannhof/cobalt-rs/build/1.0.560
Another build with 1.23, see https://ci.appveyor.com/project/johannhof/cobalt-rs/build/1.0.586

epage · 2018-01-09T18:33:15Z

If the build succeeds, feel free to squash the commits, documenting in them why you removed what you did, and let me know so I can merge this.

Geobert · 2018-01-09T19:23:39Z

https://ci.appveyor.com/project/tak1n/cobalt-rs/build/1.0.587/job/cw095bwrnbsdop0x#L976

I don't know what to do about this.

epage · 2018-01-09T19:28:29Z

We're still getting sporadic failures?

This is not fun.

Geobert · 2018-01-09T21:59:08Z

I've asked reddit for help.

epage

Looking great! Good job on seeing this through to getting it to work!

I left some comments in the appeyvor file. I'm unsure how much of these things are vestiges of previous attempts vs things we need

epage · 2018-01-11T00:22:23Z

appveyor.yml

-  - if [%APPVEYOR_REPO_TAG%]==[false] ( cargo test --verbose )
+  - if [%APPVEYOR_REPO_TAG%]==[false] ( cargo +%CHANNEL%-%TARGET% build )
+  - if [%APPVEYOR_REPO_TAG%]==[false] ( cargo +%CHANNEL%-%TARGET% test )
+  - if [%APPVEYOR_REPO_TAG%]==[false] ( cargo +%CHANNEL%-%TARGET% build --no-default-features )


Do these still need to be build instead of check?

As said on gitter: because of natives C/C++ libs we need the build as check doesn't support them yet.

The annoying thing is that this doesn't seem to be a problem on Linux :(

epage · 2018-01-11T00:22:50Z

appveyor.yml

    # Beta channel
    - TARGET: x86_64-pc-windows-msvc
      CHANNEL: beta
+      TOOLCHAIN: msvc
+      PLATFORM: x86_64


What is PLATFORM?

Should we set it globally?

Good catch, it was used in the install part to call the correct config .bat file according to i686 vs x86_64. To be deleted :)

epage · 2018-01-11T00:23:11Z

appveyor.yml

    CRATE_NAME: cobalt
    APPVEYOR_CACHE_SKIP_RESTORE: true
+    MSBUILD_NODE_CONNECTION_TIMEOUT: 0


What value does this give us?

This was one of the workaround for the PDB issue, I'll try without it :)

epage · 2018-01-11T00:24:50Z

.travis.yml

+  - cargo check --no-default-features --verbose
+  - cargo test --no-default-features --verbose
+  - cargo check --verbose --features "sass"
+  - cargo test  --verbose --features "sass"


At some point we should make it so different feature combinations run as different jobs so they can run in parallel rather than series.

Granted that can get messy, particularly when we already have problems with our rustfmt and clippy jobs (see #305)

Is this possible with travis?

Yup, we just define more jobs in the matrix with a different variable set to control what happens in the job, maybe a FEATURE variable that is --feature "sass" vs --no-default-features.

Not something to include in this because it'd require a lot more experimenting and we shouldn't hold this up :)

epage · 2018-01-11T22:24:40Z

Sweet! Squash the commits and I'll get this merged!

Thanks for all your effort on this one. Its been long and frustrating. I'm glad this didn't burn you out. Side projects should reinvigorate us and not tear us down.

- Remove windows-gnu target, support of Windows only through MSVC - Remove 32 bits support - Switch to Visual Studio 2017 to avoid PBD error Remove uneeded variables

epage reviewed Jan 2, 2018

View reviewed changes

epage reviewed Jan 3, 2018

View reviewed changes

epage mentioned this pull request Jan 3, 2018

Document "install from source" requirements cobalt-org/cobalt-org.github.io#19

Closed

epage mentioned this pull request Jan 3, 2018

main_binary's stderr may include extra text assert-rs/assert_cli#79

Closed

epage reviewed Jan 4, 2018

View reviewed changes

Geobert force-pushed the #180 branch from e856133 to 96eeafa Compare January 5, 2018 11:41

Geobert force-pushed the #180 branch from 8b468ff to 033711c Compare January 5, 2018 12:34

epage requested changes Jan 6, 2018

View reviewed changes

epage reviewed Jan 8, 2018

View reviewed changes

epage approved these changes Jan 9, 2018

View reviewed changes

Geobert mentioned this pull request Jan 9, 2018

intermittent linker errors on 32-bit MSVC builds rust-lang/cargo#4655

Closed

Geobert force-pushed the #180 branch from d824300 to 6d56d75 Compare January 10, 2018 21:35

epage requested changes Jan 11, 2018

View reviewed changes

epage approved these changes Jan 11, 2018

View reviewed changes

fixes cobalt-org#180 : Enable syntax highlight to all platform

2549bfc

- Remove windows-gnu target, support of Windows only through MSVC - Remove 32 bits support - Switch to Visual Studio 2017 to avoid PBD error Remove uneeded variables

Geobert force-pushed the #180 branch from d716d0a to 2549bfc Compare January 12, 2018 08:13

Geobert mentioned this pull request Jan 12, 2018

Can not cargo build --release on Windows 10. getzola/zola#198

Closed

epage merged commit 8003c11 into cobalt-org:master Jan 15, 2018

Geobert deleted the #180 branch January 15, 2018 19:52

fixes #180 : Enable syntax highlight to all platform #352

fixes #180 : Enable syntax highlight to all platform #352

Uh oh!

Conversation

Geobert commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jan 2, 2018

Uh oh!

Geobert commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Geobert commented Jan 2, 2018

Uh oh!

epage commented Jan 2, 2018

Uh oh!

Geobert commented Jan 2, 2018

Uh oh!

epage commented Jan 2, 2018

Uh oh!

Geobert commented Jan 2, 2018

Uh oh!

Geobert commented Jan 2, 2018

Uh oh!

epage commented Jan 2, 2018

Uh oh!

epage commented Jan 2, 2018

Uh oh!

Geobert commented Jan 2, 2018

Uh oh!

epage commented Jan 2, 2018

Uh oh!

Geobert commented Jan 3, 2018

Uh oh!

Geobert commented Jan 3, 2018

Uh oh!

epage commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epage commented Jan 3, 2018

Uh oh!

Geobert commented Jan 3, 2018

Uh oh!

epage commented Jan 3, 2018

Uh oh!

Geobert commented Jan 3, 2018

Geobert commented Jan 2, 2018 •

edited

Loading

Geobert commented Jan 5, 2018 •

edited

Loading

epage commented Jan 9, 2018 •

edited

Loading