Skip to content

Additional Unit Tests, Refactoring, & C FFI Pointer Validation #114

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

Merged
merged 14 commits into from
Jul 2, 2018

Conversation

mmstick
Copy link
Member

@mmstick mmstick commented Jun 20, 2018

There should be no changes to functionality or the API made in this PR. Functionality only needs to be tested to ensure that everything still works as before. GTK installer + CLI

  • Fix a build error due to a lack of a dev dependency for an example
  • Fix an existing test which is broken
  • Add many more unit tests for that which is not yet unit tested
  • Refactor BlockInfo
  • Add a few function comments missing from PartitionInfo's methods.
  • Applied clippy suggestions, which will improve performance
  • Refactored the CLI workspace into modules
  • Check every pointer received from the C FFI before attempting to do anything with them.

Closes #45

@mmstick mmstick changed the title Add Additional Unit Tests Add Additional Unit Tests & Refactor Jun 20, 2018
@mmstick
Copy link
Member Author

mmstick commented Jun 21, 2018

The clippy bug that distinst's source code uncovered yesterday was fixed today, so I'll soon submit another clippy commit.

There is an issue that prevents the ffi workspace from being lint-checked, however
@mmstick mmstick changed the title Add Additional Unit Tests & Refactor Additional Unit Tests, Refactoring, & C FFI Pointer Validation Jun 21, 2018
@brs17
Copy link
Contributor

brs17 commented Jul 2, 2018

Had an installation fail on me when installing with these patches.

Logs from installer:
ERROR: bootloader error: nul byte found in provided data
installerlogs.log

journald logs:
journaldlogs.log

@mmstick
Copy link
Member Author

mmstick commented Jul 2, 2018

@brs17 I think that issue is affecting master at the moment. Thought I had fixed it. Seems to happen at random, because something (I'm assuming Vala) is freeing some statically-initialized memory that distinst wants to reuse.

Short-term, I may hand Vala a unique copy of the data so it won't also end up eating the hand that feeds it in the process.

@brs17
Copy link
Contributor

brs17 commented Jul 2, 2018

Looks like while it said installation failed, it actually worked.

Huh, interesting, I'm going to give this pr another go.

@mmstick
Copy link
Member Author

mmstick commented Jul 2, 2018

About to submit a commit that should fix the issue for now.

@brs17
Copy link
Contributor

brs17 commented Jul 2, 2018

Tried the latest from this pr. After updating, initially the installer did not run when I selected it. Selecting it a second time, it launched and installed just fine. Only oddity I noticed was the titlebar:

languaging

@mmstick
Copy link
Member Author

mmstick commented Jul 2, 2018

I know what's wrong. Submitting a fix soon.

@mmstick
Copy link
Member Author

mmstick commented Jul 2, 2018

@brs17 You should be able to try again soon.

@brs17 brs17 self-requested a review July 2, 2018 20:49
Copy link
Contributor

@brs17 brs17 left a comment

Choose a reason for hiding this comment

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

Looks good from a "it doesn't break stuff" viewpoint.

@mmstick mmstick merged commit 4563f76 into master_bionic Jul 2, 2018
@mmstick mmstick deleted the unit-tests branch August 14, 2018 17:19
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