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

fix appveyor installer and build #224

Merged
merged 5 commits into from Mar 1, 2019
Merged

fix appveyor installer and build #224

merged 5 commits into from Mar 1, 2019

Conversation

xmclark
Copy link
Contributor

@xmclark xmclark commented Feb 28, 2019

This PR introduces a few changes. I reset and squashed some commits to make the intentions more clear. Here are some of the high level changes:

  • Reverted changes to how we allocate virtual memory, this may have been related to the issues we were seeing in appveyor builds.
  • We now unregister exception handlers have functions have been executed. This is naive, but it fixes issues with assertions and panics triggering the windows exception handlers when they shouldn't.
  • There were a few changes made to the appveyor file including some comments and changes to how artifacts are published, and tests executed

I have run these tests several times now, and feel confident that we should not have many more fluke failures. There is still more investigation needed...for example, spectests will fail if run singled threaded, but will pass if running multithreaded.

@xmclark xmclark changed the title fix appveyor installer fix appveyor installer and build Mar 1, 2019
@@ -33,7 +33,7 @@ impl Memory {

let protect = protection.to_protect_const();

let ptr = unsafe { VirtualAlloc(ptr::null_mut(), size, MEM_RESERVE | MEM_COMMIT, protect) };
Copy link
Contributor

Choose a reason for hiding this comment

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

This still works, even though we're not committing the memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

Approved for the 3rd time 😂

@xmclark xmclark merged commit bde2022 into master Mar 1, 2019
@xmclark xmclark deleted the fix/appveyor-installer branch March 1, 2019 21:16
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.

None yet

3 participants