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

Strip unc if it exists, fix #1110 #1129

Closed
wants to merge 0 commits into from
Closed

Strip unc if it exists, fix #1110 #1129

wants to merge 0 commits into from

Conversation

uggla
Copy link
Contributor

@uggla uggla commented Aug 14, 2020

Sanity check:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

  • Are you doing the PR on the next branch?

Explanation:
Remove unc on windows platform see #1110. Unfortunately I have no windows system at the moment to check that this patch is working as expected.
I'm opening this PR to check if the CI is ok.

@uggla
Copy link
Contributor Author

uggla commented Aug 14, 2020

Linux pined failed due to this:
error[E0658]: use of unstable library feature 'str_strip': newly added

It seems that this method used by strip_prefix was not stable in 1.41.
I'm gonna add a test to ensure init msg is fine on windows.

@Keats, despite you may not accept this PR, I would like to continue working on it because it allows me to better understand zola codebase and practise a bit of Rust. Unless if it is causing some trouble on your side.

@uggla uggla changed the title Strip unc if it exists, fix #1110 WIP:Strip unc if it exists, fix #1110 Aug 14, 2020
@uggla
Copy link
Contributor Author

uggla commented Aug 14, 2020

Looks ok regarding the strip_unc function. TODO finalize the test.

unning 11 tests
test cmd::init::tests::init_empty_directory ... ok
test cmd::init::tests::init_non_empty_directory ... ok
test cmd::init::tests::init_quasi_empty_directory ... ok
test cmd::init::tests::populate_existing_directory ... ok
test cmd::init::tests::populate_non_existing_directory ... ok
"\\\\?\\C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\new_project"
\\?\C:\Users\VssAdministrator\AppData\Local\Temp\new_project
C:\Users\VssAdministrator\AppData\Local\Temp\new_project   --> unc removed
test cmd::init::tests::strip_unc_test ... ok

@Keats
Copy link
Collaborator

Keats commented Aug 16, 2020

You can bump the min Rust version in https://github.com/getzola/zola/blob/master/azure-pipelines.yml#L24 to the new min verison

@uggla uggla changed the title WIP:Strip unc if it exists, fix #1110 Strip unc if it exists, fix #1110 Aug 20, 2020
@uggla
Copy link
Contributor Author

uggla commented Aug 20, 2020

@Keats , done on my side, if you can review.

@@ -21,7 +21,7 @@ stages:
rustup_toolchain: stable
linux-pinned:
imageName: 'ubuntu-16.04'
rustup_toolchain: 1.41.0
rustup_toolchain: 1.45.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need the latest version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not, but something in between 1.42 and 1.45.2. Do you want me to check in which version the problem is fixed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error[E0658]: use of unstable library feature 'str_strip': newly added
  --> src/cmd/init.rs:65:26
   |
65 |     match path_to_refine.strip_prefix(LOCAL_UNC) {
   |                          ^^^^^^^^^^^^
   |
   = note: see issue #67302 <https://github.com/rust-lang/rust/issues/67302> for more information

error: aborting due to previous error

For more information about this error, try `rustc --explain E0658`.
error: could not compile `zola`.

This was stabilized in 1.45, (https://github.com/rust-lang/rust/blob/master/RELEASES.md#stabilized-apis-1), I manage to confirm using rustc as well. So minimum version to bump is 1.45 to avoid this error. I'm gonna change that accordingly.

src/cmd/init.rs Outdated
@@ -25,6 +26,8 @@ build_search_index = %SEARCH%
# Put all your custom variables here
"#;

const LOCAL_UNC: &str = "\\\\?\\";
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add a comment about what this is? I will have forgotten about UNC in about a week

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
However, I saw the comments on the issue. Of course this is not the best fix, long term one will come as soon as the rust team will fix it properly. However in the mean time this is a simple workaround.
So if you want to merge this PR, I'll be happy to refine it. Otherwise you can just close it. I will not be disappointed, I did it to try to better understand the code so it is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The main thing is that I/we will never remember to check back on the Rust issue to see when it will be fixed :/

uggla added a commit to uggla/zola that referenced this pull request Aug 31, 2020
@uggla uggla closed this Aug 31, 2020
uggla added a commit to uggla/zola that referenced this pull request Aug 31, 2020
Keats pushed a commit that referenced this pull request Sep 1, 2020
* Strip unc if it exists, fix #1110

* Bump pinned version to 1.45.2

* Bump to minimum version required.

- #1129 (comment)

* Add unc comments and a required test

* Fix typo in rust pinned version

* Fix types not ok
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.

2 participants