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 efile_allocate for Mac #2386

Merged
merged 1 commit into from
Sep 19, 2019
Merged

Fix efile_allocate for Mac #2386

merged 1 commit into from
Sep 19, 2019

Conversation

indocomsoft
Copy link
Contributor

@CLAassistant
Copy link

CLAassistant commented Sep 12, 2019

CLA assistant check
All committers have signed the CLA.

@jhogberg jhogberg added the team:VM Assigned to OTP team VM label Sep 12, 2019
@jhogberg jhogberg self-assigned this Sep 12, 2019
@jhogberg jhogberg added the fix label Sep 12, 2019
@jhogberg
Copy link
Contributor

jhogberg commented Sep 13, 2019

(Continuing the discussion from ERL-1042)

Should I keep the current behaviour on Linux?
Otherwise, I can standardise the behaviour to change the file size as well, in which case I will also change the Linux-specific part of the code to have mode be 0 instead of FALLOC_FL_KEEP_SIZE, and update the documentation

Yes, we don't like introducing behavioral changes in patches but it should be changed on master later on as we strive to have the same behavior on all platforms. It should've been this way to begin with in my opinion.

Can you squash and rebase the fix to maint, and expand the test in file_SUITE to verify that the file size is updated? We can settle for only testing this on the platforms that we know update it at the moment.

@jhogberg jhogberg changed the base branch from master to maint September 13, 2019 09:55
@jhogberg jhogberg changed the base branch from maint to master September 13, 2019 10:55
@jhogberg jhogberg added the testing currently being tested, tag is used by OTP internal CI label Sep 13, 2019
@indocomsoft
Copy link
Contributor Author

I am still trying to get the test to fail when I revert the fix.

@indocomsoft
Copy link
Contributor Author

indocomsoft commented Sep 13, 2019

@jhogberg I think I'm finally able to get the test to fail when I revert the fix. This is my first time writing Erlang actually. I usually write in Elixir, so I'm not too sure about the test that I wrote in Erlang. Do let me know if there are things to be improved.

Also, should I squash everything into 1 commit?

Copy link
Contributor

@jhogberg jhogberg left a comment

Choose a reason for hiding this comment

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

The test looks good aside from a minor nit, and we'd prefer it to be in the same commit as the fix.

lib/kernel/test/file_SUITE.erl Outdated Show resolved Hide resolved
@indocomsoft
Copy link
Contributor Author

Fixed! I also followed the other tests in calling flush() and then returning ok

@jhogberg
Copy link
Contributor

jhogberg commented Sep 16, 2019

Looks good, if all goes well I'll merge it after 22.1 is released later this week. :)

@indocomsoft
Copy link
Contributor Author

Great! Looking forward to the 22.1 release.

Should I also create a PR to change the Linux behaviour based off master after this PR is merged?

@jhogberg
Copy link
Contributor

Should I also create a PR to change the Linux behaviour based off master after this PR is merged?

Feel free!

@jhogberg jhogberg merged commit 4c024f1 into erlang:master Sep 19, 2019
@jhogberg
Copy link
Contributor

Merged, thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants