Skip to content

Fix windows build #961

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

Closed
wants to merge 14 commits into from
Closed

Conversation

kureuil
Copy link
Contributor

@kureuil kureuil commented Aug 15, 2017

Windows MSVC being a Tier1 platform it would seems logical that crates.io should be able to build on it.

TODO:

PS: I'm sorry for messing with the package-lock.json but it kept complaining about package integrity issues so I had to delete it and regenerate it in order to silence it, please forgive me. If anyone has clues about it I'll gladly try (npm cache clean & npm cache verify didn't helped me as much as I hoped) Seems that updating npm to the latest version (v5.4.2) fixed the problem.

Added Windows compatible version of rust-civet. Regenerated a
package-lock.json because of sha512 integrity issues.
I did not understand why the git repository were removed each time the
`git::init` function was removed while they were created only once per
thread (?). I also re-coded a remove_dir_all function for the Windows
platform because the stdlib's one kept failing with 'Access denied' when
deleting git repositories.
@kureuil kureuil force-pushed the fix-windows-build branch 8 times, most recently from de8a5a3 to e58df41 Compare September 14, 2017 14:23
OpenSSL is now installed via pre-compiled binaries instead of vcpkg,
and libpq should be installed system-wide when asking for PostgresQL
as a service.

Also, cargo's local registry is now invalidated when the Cargo.lock
file changes, avoiding cache size issues.
@kureuil kureuil changed the title [WIP] Fix windows build Fix windows build Sep 15, 2017
@kureuil
Copy link
Contributor Author

kureuil commented Sep 15, 2017

@carols10cents This PR is now ready for review.

  • I wanted to add AppVeyor badges to the README.md and Cargo.toml files, but we might have to wait for Fix Wrong AppVeyor badge #1050 to land due to AppVeyor's non deterministic naming convention.
  • Even though it's labeled as a Windows-related PR, the civet web server was updated from 1.6 to 1.9.1. Everything seems good right now but more testing cannot harm.

If anyone has more information on what this code is supposed to do, can you please explain ?

https://github.com/rust-lang/crates.io/blob/master/src/tests/git.rs#L26-L32

From what I understand it removes the checkout and bare folders each time the function is called but creates them only once, when the program is launched.

@carols10cents
Copy link
Member

If anyone has more information on what this code is supposed to do, can you please explain ?

https://github.com/rust-lang/crates.io/blob/master/src/tests/git.rs#L26-L32

From what I understand it removes the checkout and bare folders each time the function is called but creates them only once, when the program is launched.

Yes, that's what this function does. It's only called in the tests, for each app in each test. What else would you like to know about it?

@kureuil
Copy link
Contributor Author

kureuil commented Sep 15, 2017

Yes, that's what this function does. It's only called in the tests, for each app in each test. What else would you like to know about it?

I don't understand why it would delete the folders each time the function is called (at least once for each test, so ~140 calls) but only create them once. Reading git2-rs's documentation, the Repository::init documentation states that will create any needed folder while Repository::init_bare documentation states that the folder in which the repository will be created must exist prior to invoking the function. But the old code was only deleting the folders and never creating the bare repo one ? I guess that git2-rs documentation is wrong and that it will create any needed folder for both init and init_bare given that the tests are running without any problems ?

Following this logic I don't understand why the folders were created in the first place ? I guess its safe to delete...

@carols10cents
Copy link
Member

carols10cents commented Oct 10, 2017

Sorry this has taken me so long to get to :( I'm having issues building civet-sys 0.2.0 on macos, going to file an issue over there....

EDIT: bug filed: wycats/rust-civet#12

Instead of simply assuming that files are immediately deleted, we move them and then mark them
for deletion. This makes deleting a folder slower but much more reliable.

For more information, see Niall Douglas intervention at CppCon 2015
@kureuil
Copy link
Contributor Author

kureuil commented Nov 1, 2017

So, I've been thinking a lot about this and I'm not sure I want this PR to be merged. However, I'd like to use this opportunity to define the platform support of the crates.io project: should we support running the application on Windows/macOS/Linux or should we only support Linux as a target and tell people to use our newly introduced docker support (#1106) to develop the application locally ?

I think we need to have an answer to this question because of its consequences. If we tell people that we only officially support running the application on Linux, then maybe we shouldn't have to care about its dependency not building on macOS. However, if we tell people that we will support all three major OS, then we need to step up our CI infrastructure to make sure that we don't accidentally break the project on either Windows or macOS and that our future development will have to be compatible with all three major OS which might complicate things given that in the end, the application will probably only run on Linux when used in production.

Finally, it would seem that even though the project isn't so young anymore, there hasn't been any issue about the backend not building on Windows MSVC, although it is not documented anywhere in the setup docs. Worse, it is even assumed that the backend can currently be build on Windows MSVC, although it never was possible due to its, now outdated, civetweb dependency. Which means that it might not be that much of a big deal, and it would seem that we haven't drove off any potential contributor. Moreover, with the imminent release of the Windows Subsystem for Linux, any person with Windows 10 FCU would be able to get this project running without us doing anything except requiring contributors to use the WSL.

tl;dr I'm mostly worried that due to its current & past inactivity, the Windows support for the backend might not get enough contribution to justify its integration.

Currently blocked on wycats/rust-civet#13.

@jtgeibel
Copy link
Member

jtgeibel commented Nov 3, 2017

I'm +1 for pushing people toward docker and/or WSL. Both seem like reasonable paths towards development on Windows.

@carols10cents
Copy link
Member

For entirely selfish reasons, I don't want to break the ability to develop natively on macos; I prefer not to use docker :)

I agree that until we get more Windows contributors, the docker support for windows is probably sufficient.

I'm going to close this. Thanks for your work!

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