Make file_info record into a native record#648
Open
garazdawi wants to merge 11 commits into
Open
Conversation
Reviewer's GuideConvert the file_info (and file_descriptor) record from a header-defined record into a native record in file.erl, adapt callers and tooling to use the new native record mechanism (import_record/include paths), ensure distribution and external term format support for native records, and adjust build flags/paths so non-kernel apps can compile against kernel’s native records. Sequence diagram for prim_file read_handle_info using native file_infosequenceDiagram
participant Caller
participant prim_file
participant NIF as prim_file_NIF
Caller->>prim_file: read_handle_info_1(Fd, TimeType)
prim_file->>prim_file: get_fd_data(Fd)
prim_file->>NIF: read_handle_info_nif(FRef)
alt NIF error
NIF-->>prim_file: {error, Reason}
prim_file-->>Caller: {error, Reason}
else NIF returns tuple_file_info
NIF-->>prim_file: {file_info,S,T,A,AT,MT,CT,M,L,MAD,MID,I,U,G}
prim_file->>prim_file: adjust_times({file_info,S,T,A,AT,MT,CT,M,L,MAD,MID,I,U,G}, TimeType)
prim_file->>prim_file: convert to #file_info{...}
prim_file->>prim_file: adjust_times(#file_info{...}, TimeType)
prim_file-->>Caller: {ok, #file_info{...}}
end
Class diagram for native file_info and file_descriptor recordsclassDiagram
class file {
+#file_info file_info
+#file_descriptor file_descriptor
}
class file_info {
+non_neg_integer size
+device|directory|other|regular|symlink|undefined type
+read|write|read_write|none|undefined access
+file_date_time_or_epoch atime
+file_date_time_or_epoch mtime
+file_date_time_or_epoch ctime
+non_neg_integer_or_undefined mode
+non_neg_integer_or_undefined links
+non_neg_integer_or_undefined major_device
+non_neg_integer_or_undefined minor_device
+non_neg_integer_or_undefined inode
+non_neg_integer_or_undefined uid
+non_neg_integer_or_undefined gid
}
class file_descriptor {
+module module
+term data
}
class kernel_file_hrl {
+import_record file[file_info,file_descriptor]
}
class bootstrap_kernel_file_hrl {
+import_record file[file_info,file_descriptor]
}
class client_module {
+include file_hrl
+use_record file_info
+use_record file_descriptor
}
file <|.. file_info : defines
file <|.. file_descriptor : defines
kernel_file_hrl ..> file : import_record
bootstrap_kernel_file_hrl ..> file : import_record
client_module ..> kernel_file_hrl : include
client_module ..> file_info : uses
client_module ..> file_descriptor : uses
class file_date_time_or_epoch {
}
class non_neg_integer_or_undefined {
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The switch from
-include_lib("kernel/include/file.hrl")to-include("file.hrl")in modules outsidekernel(e.g. ssh/ssl/ftp/tftp/tools/etc.) introduces explicit-I ../../kernel/includecoupling in multiple Makefiles; consider either keepinginclude_libfor cross-application users or centralizing this include-path configuration to avoid fragile relative-path dependencies. - In
prim_file:adjust_times/2, the long tuple pattern for{file_info, ...}hard-codes the field order; it may be safer and easier to maintain to centralize the tuple-to-#file_info{}conversion (or use a small helper) so changes to the record layout only need to be updated in one place. - Double-check that all places where
-include("file.hrl")is now used (notably undererts/preloaded/src/) have the appropriate include paths configured at build time, as these were previously resolved viainclude_liband may fail iffile.hrlis not on the compiler include path for those targets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The switch from `-include_lib("kernel/include/file.hrl")` to `-include("file.hrl")` in modules outside `kernel` (e.g. ssh/ssl/ftp/tftp/tools/etc.) introduces explicit `-I ../../kernel/include` coupling in multiple Makefiles; consider either keeping `include_lib` for cross-application users or centralizing this include-path configuration to avoid fragile relative-path dependencies.
- In `prim_file:adjust_times/2`, the long tuple pattern for `{file_info, ...}` hard-codes the field order; it may be safer and easier to maintain to centralize the tuple-to-`#file_info{}` conversion (or use a small helper) so changes to the record layout only need to be updated in one place.
- Double-check that all places where `-include("file.hrl")` is now used (notably under `erts/preloaded/src/`) have the appropriate include paths configured at build time, as these were previously resolved via `include_lib` and may fail if `file.hrl` is not on the compiler include path for those targets.
## Individual Comments
### Comment 1
<location> `erts/preloaded/src/prim_file.erl:684-690` </location>
<code_context>
error:_ -> {error, badarg}
end.
+adjust_times({file_info, S, T, A, AT, MT, CT, M, L,
+ MAD, MID, I, U, G}, TimeType) ->
+ adjust_times(#file_info{size = S, type = T, access = A,
+ atime = AT, mtime = MT, ctime = CT,
+ mode = M, links = L,
+ major_device = MAD, minor_device = MID, inode = I,
+ uid = U, gid = G}, TimeType);
adjust_times(FileInfo, posix) ->
FileInfo;
</code_context>
<issue_to_address>
**issue (bug_risk):** The new #file_info construction requires the record definition to be visible in prim_file.
This clause now uses the `#file_info{}` record, but `prim_file.erl` doesn’t appear to include its definition (previously it only handled the tuple/map from the NIF). Without something like `-include("file.hrl").` (or equivalent) the module won’t compile. Please make the record definition available here, consistent with other callers of `#file_info{}`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+684
to
+690
| adjust_times({file_info, S, T, A, AT, MT, CT, M, L, | ||
| MAD, MID, I, U, G}, TimeType) -> | ||
| adjust_times(#file_info{size = S, type = T, access = A, | ||
| atime = AT, mtime = MT, ctime = CT, | ||
| mode = M, links = L, | ||
| major_device = MAD, minor_device = MID, inode = I, | ||
| uid = U, gid = G}, TimeType); |
There was a problem hiding this comment.
issue (bug_risk): The new #file_info construction requires the record definition to be visible in prim_file.
This clause now uses the #file_info{} record, but prim_file.erl doesn’t appear to include its definition (previously it only handled the tuple/map from the NIF). Without something like -include("file.hrl"). (or equivalent) the module won’t compile. Please make the record definition available here, consistent with other callers of #file_info{}.
There was a problem hiding this comment.
Pull request overview
This pull request converts the file_info and file_descriptor records from traditional Erlang records to native records. The migration involves:
- Moving record definitions from
lib/kernel/include/file.hrltolib/kernel/src/file.erlusing the-export_recorddirective - Updating
file.hrlto use-import_recordto import the records from thefilemodule - Changing all module includes from
-include_lib("kernel/include/file.hrl")to-include("file.hrl")across multiple OTP libraries - Adding
-I../../kernel/includeor similar include paths to Makefiles to ensurefile.hrlcan be found - Updating
prim_file.erlto handle tuple-to-record conversion for compatibility - Modifying
external.cto pass theflagsparameter when decoding atoms in native records (for atom cache support) - Adding a new test case in
native_record_SUITEfor distribution testing - Updating bootstrap beam files
Changes:
- Record definitions migrated from include file to module with export/import directives
- Include directives changed from
include_libto plainincludeacross ~50 files - Makefile updates to add kernel include path to compilation flags
- Native record support for distribution with atom cache
Reviewed changes
Copilot reviewed 85 out of 118 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/kernel/include/file.hrl | Replaced record definitions with -import_record directive |
| lib/kernel/src/file.erl | Added record definitions with -export_record directive |
| erts/preloaded/src/prim_file.erl | Added tuple-to-record conversion in adjust_times function |
| erts/emulator/beam/external.c | Updated atom decoding to pass flags for atom cache support |
| erts/emulator/beam/dist.c | Added DOP_ALTACT_SIG_SEND constant string |
| erts/emulator/test/native_record_SUITE.erl | Added distribution test for native records |
| lib/*/src/Makefile | Added -I../../kernel/include to compilation flags |
| lib//src/.erl (multiple) | Changed includes from include_lib to include |
| bootstrap/* | Updated bootstrap beam files for new record format |
| erts/preloaded/src/Makefile | Added conditional around dep.mk include |
| .github/workflows/main.yaml | Removed check-beam dependency from pack job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| pack: | ||
| name: Build Erlang/OTP (64-bit) | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
The removal of the needs: check-beam dependency from the pack job appears to be unrelated to the main purpose of this PR (converting file_info to a native record). This change allows the pack job to run in parallel with the check-beam job rather than waiting for it to complete.
While this might improve CI performance, it's unclear from the PR description why this change is included. If this is intentional and related to the native record migration (perhaps because bootstrap beam files need to be updated), it should be mentioned in the PR description. Otherwise, this change should be in a separate PR focused on CI improvements.
We do this in order to be able to use a OTP release where file info is a tuple records to compile this version is is going to use native-records.
We don't have a working erlang system at this point so we cannot create the dep file.
garazdawi
force-pushed
the
lukas/kernel/native-file_info
branch
from
82d3e87 to
fd442cc
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order for this migration to really work the lists:key functions needs to work with native records.
Summary by Sourcery
Convert the file-related records to native records and propagate their usage across the codebase, while fixing distribution handling and build flags to support the new representation.
New Features:
Bug Fixes:
Enhancements:
CI: