Windows Compatibility Fixes #1 #2535

yeastplume · 2019-02-06T11:48:46Z

Meta Issue: #2525

After this PR, Grin should compile, run and sync on windows (with remaining problems, see #2525 for instructions and remaining issues). In particular, this:

Updates croaring-rs from 0.3 to 0.3.8
Remove daemonize and grin server stop|start commands
Update inclusion of cursive to be platform conditional, change terminal backend for Windows to pancurses
Updates to PMMR (particularly file rewind) to allow for explicit dropping of file handles. Required due to stricter locking on windows
Modifications to store and wallet tests (which also need to explicitly drop file handles in many cases)
Temporarily reduce initial LMDB file storage on windows (more complete solution needed, see (GrinWin) - Windows 10 Support Meta-Issue #2525)
Add functions to explicitly drop underlying file handles in TxHashSet. Not doing so was preventing fast-sync archives from unzipping.

These changes definitely need to be reviewed and tested to ensure they don't break anything on linux

yeastplume · 2019-02-06T11:50:15Z

store/src/pmmr.rs

-	hash_file: DataFile<Hash>,
-	data_file: DataFile<T::E>,
+	hash_file: Option<DataFile<Hash>>,
+	data_file: Option<DataFile<T::E>>,


Options so they can be dropped before renaming operations

May be cleaner by pushing this down to DataFile and AppendOnlyFile.

yeastplume · 2019-02-06T11:51:40Z

store/src/types.rs

+				.read(true)
+				.create(true)
+				.append(true)
+				.open(&self.path)?;


File permissions for set_len on windows need to be as set here or it fails. Hence the drop and reopen with original permissions after the set_len.

ignopeverell

Overeall looking good but as you're already doing a fair amount of handling in AppendOnlyFile, it looks like a lot of the recreating could be pushed down there as well, avoiding the multiple Option hassle.

ignopeverell · 2019-02-07T20:29:59Z

src/bin/cmd/server.rs

+
+//TODO: This
+#[cfg(target_os = "windows")]
+fn daemonize(config: servers::ServerConfig) {


I've been pondering removing the use of daemonize and start/stop altogether. Unixes provide plenty of ways to do this and there are a couple ways on Windows as well. People are likely to want to do it in all sorts of different ways as well.

ignopeverell · 2019-02-07T20:57:51Z

store/src/pmmr.rs

-	hash_file: DataFile<Hash>,
-	data_file: DataFile<T::E>,
+	hash_file: Option<DataFile<Hash>>,
+	data_file: Option<DataFile<T::E>>,


May be cleaner by pushing this down to DataFile and AppendOnlyFile.

ignopeverell · 2019-02-07T20:59:06Z

store/src/pmmr.rs

@@ -334,20 +349,24 @@ impl<T: PMMRable> PMMRBackend<T> {
 			}
 			self.prune_list.flush()?;
 		}
-
+		// drop hash file, or windows will still have a lock on it and won't allow the rename


If pushed down, then the whole remove, rename and reallocate can be encapsulated in some reopen operation in AppendOnlyFile?

i1skn · 2019-02-07T23:11:14Z

store/src/lmdb.rs

+	//TODO: This is temporary to support (beta) windows support
+	//Windows allocates the entire file at once, so this needs to
+	//be changed to allocate as little as possible and increase as needed
+	#[cfg(target_os = "windows")]


Can we add cfg!(target_os = "ios") here as well?

I'd rather just keep this PR to windows issues for now, and this is just temporary. The real fix shouldn't involve OS specific code, but it should allocate the file minimally and increase it as necessary.

ok, got it! let me know if I can help somehow with this upcoming fix!

yeastplume · 2019-02-08T11:29:25Z

@ignopeverell Thanks, cleaned that up and encapsulated the file rename/replace. However, needs slightly more review given that I had to add functionality to drop underlying PMMR files from the txhashset during fast sync archive unzip. Marked above.

yeastplume · 2019-02-08T11:30:15Z

chain/src/chain.rs

+		{
+			let mut txhashset_ref = self.txhashset.write();
+			// Drop file handles in underlying txhashset
+			txhashset_ref.release_backend_files();


Seems to work, would just like a bit of reassurance that this isn't going to have any odd side effects.

Not that I can think of, seems fair.

yeastplume · 2019-02-08T12:22:09Z

Also, removed Daemonize altogether, think I agree it's far easier to use external commands to run something as a service in most cases

ignopeverell

Very nice, all looking good to me!

ignopeverell · 2019-02-09T19:05:27Z

chain/src/chain.rs

+		{
+			let mut txhashset_ref = self.txhashset.write();
+			// Drop file handles in underlying txhashset
+			txhashset_ref.release_backend_files();


Not that I can think of, seems fair.

* - Add backwards compatability - Add hex serialization * rustfmt * rustfmt * Windows Compatibility Fixes #1 (#2535) * initial changes for windows build and unit/integration tests * rustfmt * wallet+store tests * rustfmt * fix linux daemonize * better encapsulate file rename * rustfmt * remove daemonize commands * rustfmt * remove server start/stop commands * add ability to drop pmmr backend files explicitly for txhashset unzip * rustfmt * fix pmmr tests * rustfmt * Windows TUI Fix (#2555) * switch pancurses backend to win32 * revert changes to restore test * compatibility fix + debug messages * rustfmt * Add content disposition for OK responses (#2545) * Testing http send and fixing accordingly * add repost method into wallet owner api (#2553) * add repost method into wallet owner api * rustfmt * Add ability to compare selection strategies (#2516) Before tx creation user can estimate fee and locked amount with different selection strategies by providing `-e` flag for `wallet send` command.

yeastplume added 4 commits February 6, 2019 10:20

initial changes for windows build and unit/integration tests

e7b186b

rustfmt

8b59c09

wallet+store tests

2eacd5c

rustfmt

8206b62

yeastplume requested review from ignopeverell and antiochp February 6, 2019 11:49

yeastplume commented Feb 6, 2019

View reviewed changes

fix linux daemonize

be2c36a

ignopeverell approved these changes Feb 7, 2019

View reviewed changes

i1skn reviewed Feb 7, 2019

View reviewed changes

yeastplume added 9 commits February 8, 2019 09:31

better encapsulate file rename

c724083

rustfmt

2fe320d

remove daemonize commands

1e606f9

rustfmt

8288a82

remove server start/stop commands

d92877d

add ability to drop pmmr backend files explicitly for txhashset unzip

1135196

rustfmt

af847fc

fix pmmr tests

7463a61

rustfmt

58874f6

yeastplume commented Feb 8, 2019

View reviewed changes

yeastplume added the enhancement label Feb 8, 2019

ignopeverell approved these changes Feb 9, 2019

View reviewed changes

ignopeverell added this to the 1.1.0 milestone Feb 9, 2019

yeastplume merged commit 15c7a27 into mimblewimble:master Feb 9, 2019

0xmichalis mentioned this pull request Feb 9, 2019

Implementing stop and status verbs for the server command #2433

Closed

0xmichalis mentioned this pull request Feb 24, 2019

Grin server fails as daemon and cannot even be backgrounded #2589

Closed

yeastplume deleted the windows_1 branch March 4, 2019 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Compatibility Fixes #1 #2535

Windows Compatibility Fixes #1 #2535

yeastplume commented Feb 6, 2019 •

edited

Loading

yeastplume Feb 6, 2019

ignopeverell Feb 7, 2019

yeastplume Feb 6, 2019

ignopeverell left a comment

ignopeverell Feb 7, 2019

ignopeverell Feb 7, 2019

ignopeverell Feb 7, 2019

i1skn Feb 7, 2019

yeastplume Feb 8, 2019

i1skn Feb 8, 2019

yeastplume commented Feb 8, 2019

yeastplume Feb 8, 2019

ignopeverell Feb 9, 2019

yeastplume commented Feb 8, 2019

ignopeverell left a comment

ignopeverell Feb 9, 2019

Windows Compatibility Fixes #1 #2535

Windows Compatibility Fixes #1 #2535

Conversation

yeastplume commented Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ignopeverell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeastplume commented Feb 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeastplume commented Feb 8, 2019

ignopeverell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeastplume commented Feb 6, 2019 •

edited

Loading