-
Notifications
You must be signed in to change notification settings - Fork 949
Write tests for rust_install::component #45
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
Conversation
std::thread::sleep_ms has been deprecated in favor of std::thread::sleep with a std::time::Duration being passed as the argument. This updates all usage in this project of sleep_ms with sleep.
Fix deprecation warnings for sleep_ms
Basic tests for installation and uninstallation. Fixed some bugs. Added validation of package installer versions. Added validation of installed metadata format, bringing it closer inline with what rust-installer produces. Added permission setting on unix.
You may want to make sure that these are running on both appveyor/travis, I think the top-level Cargo tests are being run but perhaps not the It also may make sense to start out these tests as integration tests in |
} | ||
pub fn init(prefix: InstallPrefix) -> Result<Self> { | ||
try!(utils::write_file("components", &prefix.manifest_file(COMPONENTS_FILE), "")); | ||
Ok(Components { prefix: prefix }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why init
was removed? The new
-> open
and Option -> Result changes make sense, although the other "InstallPrefix wrappers" for lack of a better term should probably be refactored in the same way (Configuration and Manifestation). init
was required for initializing an fresh install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I thought I left a comment about this. It's because initializing the components
file before any components are written, and outside of a transaction, could leave the installation in a partial state where nothing is installed but there is a rustlib/components file. Both 'components' and 'rust-installer-version' are initialized inside the transaction that installs the first component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change Configuration and manifestation because I haven't started learning that code yet, but I can also change them today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem - it was meant more as a note that we'll have to do it than as a criticism of the PR!
Thanks for the review @Diggsey |
Here's the rustfmt PR against master. I've updated this PR to move the tests to the tests directory and run rustfmt over it. I'm going to continue writing tests for the transaction code now and will keep appending commits to this PR, then I will move on to learning the manifest code. |
Incredible progress! I think @alexcrichton is right though: none of the |
Run rustfmt over everything
Conflicts: rust-install/src/errors.rs rust-install/src/install.rs rust-install/src/notify.rs rust-install/src/utils/mod.rs rust-install/src/utils/raw.rs
Hm, the history on this PR looks weird with some other folks commits. I must have merged something wrong. Edit: the extra commits are because this PR is against branch 'new' and I merged from 'master'. |
Uh oh. rustfmting everything made the combined diff useless... |
OK, I've addressed all the feedback. I'm not going to push more work to this branch because of the big diff already. The appveyor build is having trouble linking openssl. Here are some of the errors:
It's using openssl from http://www.npcglib.org/~stathis/downloads/openssl-1.0.2d-vs2015.7z |
I think that may have to do with some of the old support in appveyor.yml. I believe this can now be fixed by emitting a [target.x86_64-pc-windows-msvc.openssl]
rustc-link-search = ['C:\OpenSSL\openssl-1.0.2d-vs2015\lib64']
rustc-link-libs = ['static=ssleay32MT', 'static=libeay32MT']
include = ['C:\OpenSSL\openssl-1.0.2d-vs2015\include']
[target.i686-pc-windows-msvc.openssl]
rustc-link-search = ['C:\OpenSSL\openssl-1.0.2d-vs2015\lib']
rustc-link-libs = ['static=ssleay32MT', 'static=libeay32MT']
include = ['C:\OpenSSL\openssl-1.0.2d-vs2015\include']
# ... etc I think the GNU triples may also need some things there as well. Basically to statically link OpenSSL on Windows I think you'll want to use |
I've opened a PR, but it looks like the |
Fix linkage errors on appveyor
Thanks alex. |
OK, I tried to fix appveyor by adding |
Write tests for rust_install::component
\o/ |
Thanks Diggsey! |
Basic tests for installation and uninstallation.
Fixed some bugs. Added validation of package installer versions.
Added validation of installed metadata format, bringing it closer
inline with what rust-installer produces. Added permission setting
on unix.
Tabs vs. spaces is screwy here. I'll submit a follow up that rustfmts master, merges master back into new, and rustfmts new.
I'm planning on writing tests for file system transactions next, then investigating the manifest code.
It's possible this branch will start diverging a lot from the master branch soon, and it'll get harder to keep them in sync. I'm not sure yet whether it's best to try to maintain continuity with the v1 manifest world, or consider this a new product and jettison the legacy stuff.