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

Refactor Build_log module #98

Merged
merged 2 commits into from
Oct 17, 2022
Merged

Conversation

MisterDA
Copy link
Contributor

@MisterDA MisterDA commented Jan 5, 2022

Sorry, I closed #90 by mistake.

On Windows, one cannot move a directory if it has files opened inside. A similar case occurs with ZFS. Ensure that all Build_log file descriptors are closed before promoting the temporary build directory and the log file it contains as definitive build result.
We can either ensure that all tailers have finished reading before closing the log file and moving the directory, or pause the tailers and resume reading from the moved log file.

I'm still struggling working on it, it's not easy, I think I always end up in some sort of deadlock where the tailers and the Db_store layer are waiting on each other reciprocally.
There's just a little refactoring before my attempt (that I haven't pushed yet).

@MisterDA MisterDA force-pushed the build-log branch 2 times, most recently from 672db73 to b474c10 Compare January 6, 2022 16:43
@patricoferris
Copy link
Contributor

patricoferris commented Jan 7, 2022

Starting to review this PR, thanks @MisterDA !

I added a test for a very simple spec file that fails on ZFS (so I presume windows too iiuc) with the expected failure and the spec file -- now I'll look through the code to see why it's not working :))

EDIT: As the original comment points out, this PR currently does not have the fix so the failure is expected, my bad :))

@MisterDA MisterDA changed the title Close all Build_log file descriptors just after sandbox finishes Refactor Build_log module Sep 9, 2022
@MisterDA
Copy link
Contributor Author

MisterDA commented Sep 9, 2022

Changed the PR just to refactor the Build_log module and opened #118 to track the original issue.

@MisterDA MisterDA marked this pull request as ready for review September 9, 2022 09:33
Copy link
Member

@tmcgilchrist tmcgilchrist left a comment

Choose a reason for hiding this comment

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

Just 2 small fixes on the pattern matches.

lib/build_log.ml Outdated Show resolved Hide resolved
lib/build_log.ml Outdated Show resolved Hide resolved
@MisterDA
Copy link
Contributor Author

Just 2 small fixes on the pattern matches.

Applied, thanks.

@tmcgilchrist tmcgilchrist merged commit af1fa85 into ocurrent:master Oct 17, 2022
@MisterDA MisterDA deleted the build-log branch October 17, 2022 07:02
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.

3 participants