Michailf netstandard2.0 for atomic file tests#6102
Michailf netstandard2.0 for atomic file tests#6102
Conversation
Removed conditional property group for output path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds .NET Standard 2.0 support to NLog.Targets.AtomicFile by introducing a NETSTANDARD Windows CreateFile-based stream path with P/Invoke bindings, updating project target frameworks and test project references, and adding a test invocation for netstandard2.0 in the Windows test script. Changes
Sequence Diagram(s)sequenceDiagram
participant AtomicTarget as AtomicFileTarget
participant CreateWinStream as CreateWindowsStream
participant NativeMethods as NativeMethods (P/Invoke)
participant Kernel32 as Kernel32
participant FileStream as FileStream
AtomicTarget->>CreateWinStream: Request stream(path, mode)
CreateWinStream->>NativeMethods: CreateFile(path, access, share, ...)
NativeMethods->>Kernel32: P/Invoke CreateFile
Kernel32-->>NativeMethods: SafeFileHandle
NativeMethods-->>CreateWinStream: SafeFileHandle
CreateWinStream->>FileStream: Wrap handle into FileStream
FileStream-->>AtomicTarget: Return FileStream
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs (1)
82-84: Enum member naming is misleading (non-blocking).
FILE_GENERIC_READ(0x80000000) andFILE_GENERIC_WRITE(0x40000000) are actuallyGENERIC_READandGENERIC_WRITEWindows constants. TheFILE_GENERIC_*names typically refer to different composite values. This works correctly for the current usage but could cause confusion if the enum is extended.Optional: Rename for clarity
- FILE_GENERIC_READ = 0x80000000, - FILE_GENERIC_WRITE = 0x40000000, - FILE_GENERIC_EXECUTE = 0x20000000, + GENERIC_READ = 0x80000000, + GENERIC_WRITE = 0x40000000, + GENERIC_EXECUTE = 0x20000000,This would require updating references in
AtomicFileTarget.csaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs` around lines 82 - 84, The enum member names FILE_GENERIC_READ/FILE_GENERIC_WRITE/FILE_GENERIC_EXECUTE are misleading; rename them to GENERIC_READ, GENERIC_WRITE, GENERIC_EXECUTE in the enum definition to match Windows constants and update all usages (e.g., references in AtomicFileTarget.cs) to the new identifiers; ensure any XML-doc/comments are adjusted and run a build to catch any remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs`:
- Around line 152-155: The condition testing whether the incoming FileMode is
Append is incorrect because FileMode is not a [Flags] enum; replace the bitwise
check "(fileMode & FileMode.Append) != 0" with an equality comparison "fileMode
== FileMode.Append" (or an explicit set of allowed modes if multiple modes
should be treated as append) in the method using the local variable fileMode so
that the branch that adds NativeMethods.Win32FileAccess.FILE_APPEND_DATA only
runs for the actual FileMode.Append case.
- Around line 198-199: CreateUnixStream is declared static but calls the
instance member CreateDirs (inherited from FileTarget), which causes a compile
error; fix by making CreateUnixStream an instance method (remove the static
modifier) or change its signature to accept the AtomicFileTarget/FileTarget
instance and update all callers accordingly so the method can call CreateDirs;
update the declaration of CreateUnixStream and any invocations to match the
chosen approach.
In `@tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj`:
- Around line 32-40: The ProjectReference and the added Reference can both be
active when TargetFramework != 'net8.0' and TestTargetFramework is set, causing
duplicate assembly references; update the ProjectReference ItemGroup (the
ProjectReference element) to include a complementary Condition so it is skipped
when the Reference is used (i.e., when TestTargetFramework != '' and
TargetFramework != 'net8.0'), ensuring only one of Reference or ProjectReference
is included; modify the Condition on the ProjectReference ItemGroup to be the
inverse of the new Reference's Condition so duplicates are avoided.
---
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs`:
- Around line 82-84: The enum member names
FILE_GENERIC_READ/FILE_GENERIC_WRITE/FILE_GENERIC_EXECUTE are misleading; rename
them to GENERIC_READ, GENERIC_WRITE, GENERIC_EXECUTE in the enum definition to
match Windows constants and update all usages (e.g., references in
AtomicFileTarget.cs) to the new identifiers; ensure any XML-doc/comments are
adjusted and run a build to catch any remaining references.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
run-tests.ps1src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/NativeMethods.cssrc/NLog.Targets.AtomicFile/NLog.Targets.AtomicFile.csprojtests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj
| if ((fileMode & FileMode.Append) != 0) | ||
| { | ||
| dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA; | ||
| } |
There was a problem hiding this comment.
Incorrect use of bitwise AND on non-flags enum.
FileMode is not a [Flags] enum, so using bitwise & can produce incorrect results. For example, FileMode.Create (2) & FileMode.Append (6) = 2, which would incorrectly pass this check.
Use equality comparison instead:
Proposed fix
- if ((fileMode & FileMode.Append) != 0)
+ if (fileMode == FileMode.Append)
{
dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((fileMode & FileMode.Append) != 0) | |
| { | |
| dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA; | |
| } | |
| if (fileMode == FileMode.Append) | |
| { | |
| dwDesiredAccess |= NativeMethods.Win32FileAccess.FILE_APPEND_DATA; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 152 - 155, The
condition testing whether the incoming FileMode is Append is incorrect because
FileMode is not a [Flags] enum; replace the bitwise check "(fileMode &
FileMode.Append) != 0" with an equality comparison "fileMode == FileMode.Append"
(or an explicit set of allowed modes if multiple modes should be treated as
append) in the method using the local variable fileMode so that the branch that
adds NativeMethods.Win32FileAccess.FILE_APPEND_DATA only runs for the actual
FileMode.Append case.
| #if !NETFRAMEWORK && !WINDOWS | ||
| private Stream CreateUnixStream(string filePath) | ||
| private static Stream CreateUnixStream(string filePath) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the CreateUnixStream method and see how it uses CreateDirs
echo "=== CreateUnixStream method context (lines 198-220) ==="
sed -n '198,220p' src/NLog.Targets.AtomicFile/AtomicFileTarget.cs
echo ""
echo "=== Search for CreateDirs property definition ==="
rg -n "CreateDirs" src/NLog.Targets.AtomicFile/ -A 2 -B 2
echo ""
echo "=== Search for CreateDirs in NLog.Targets (FileTarget base class) ==="
rg -n "CreateDirs\s*\{|CreateDirs\s*=>" --type cs | head -20Repository: NLog/NLog
Length of output: 1937
🏁 Script executed:
#!/bin/bash
# Verify AtomicFileTarget inheritance and check if there's a way it could work
echo "=== Check AtomicFileTarget class definition ==="
head -50 src/NLog.Targets.AtomicFile/AtomicFileTarget.cs | grep -A 5 "class AtomicFileTarget"
echo ""
echo "=== Check if CreateUnixStream is called from instance context ==="
rg -n "CreateUnixStream" src/NLog.Targets.AtomicFile/ -B 3 -A 3Repository: NLog/NLog
Length of output: 1425
🏁 Script executed:
#!/bin/bash
# Check the method containing the CreateUnixStream call at line 115
echo "=== Method containing CreateUnixStream call (lines 105-125) ==="
sed -n '105,125p' src/NLog.Targets.AtomicFile/AtomicFileTarget.cs
echo ""
echo "=== Verify CreateDirs property in FileTarget ==="
sed -n '95,105p' src/NLog/Targets/FileTarget.csRepository: NLog/NLog
Length of output: 1650
🏁 Script executed:
#!/bin/bash
# Check the full method context where CreateUnixStream is called (lines 80-125)
echo "=== Full method containing CreateUnixStream call ==="
sed -n '80,125p' src/NLog.Targets.AtomicFile/AtomicFileTarget.cs | head -60Repository: NLog/NLog
Length of output: 2176
Remove static keyword or pass this instance to CreateUnixStream.
CreateUnixStream is declared static but references CreateDirs at line 206—an instance property inherited from FileTarget. Static methods cannot access instance members. This will cause a compilation error. Either remove the static modifier or refactor to pass the instance as a parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NLog.Targets.AtomicFile/AtomicFileTarget.cs` around lines 198 - 199,
CreateUnixStream is declared static but calls the instance member CreateDirs
(inherited from FileTarget), which causes a compile error; fix by making
CreateUnixStream an instance method (remove the static modifier) or change its
signature to accept the AtomicFileTarget/FileTarget instance and update all
callers accordingly so the method can call CreateDirs; update the declaration of
CreateUnixStream and any invocations to match the chosen approach.
| <ItemGroup Condition=" '$(TargetFramework)' != 'net8.0' AND '$(TestTargetFramework)' != '' "> | ||
| <Reference Include="NLog.Targets.AtomicFile"> | ||
| <HintPath>../../src/NLog.Targets.AtomicFile/bin/$(Configuration)/$(TestTargetFramework)/NLog.Targets.AtomicFile.dll</HintPath> | ||
| <Private>True</Private> | ||
| </Reference> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <ProjectReference Include="..\..\src\NLog.Targets.AtomicFile\NLog.Targets.AtomicFile.csproj" /> |
There was a problem hiding this comment.
Potential duplicate reference when TestTargetFramework is set.
The new Reference (lines 33-36) and the existing ProjectReference (line 40) can both be active when TargetFramework != 'net8.0' and TestTargetFramework != ''. This may cause duplicate assembly warnings or unexpected behavior.
Consider adding a complementary condition to the ProjectReference to exclude it when the direct reference is used:
Proposed fix
- <ItemGroup>
+ <ItemGroup Condition=" '$(TargetFramework)' == 'net8.0' OR '$(TestTargetFramework)' == '' ">
<ProjectReference Include="..\..\src\NLog.Targets.AtomicFile\NLog.Targets.AtomicFile.csproj" />
</ItemGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/NLog.Targets.AtomicFile.Tests/NLog.Targets.AtomicFile.Tests.csproj`
around lines 32 - 40, The ProjectReference and the added Reference can both be
active when TargetFramework != 'net8.0' and TestTargetFramework is set, causing
duplicate assembly references; update the ProjectReference ItemGroup (the
ProjectReference element) to include a complementary Condition so it is skipped
when the Reference is used (i.e., when TestTargetFramework != '' and
TargetFramework != 'net8.0'), ensuring only one of Reference or ProjectReference
is included; modify the Condition on the ProjectReference ItemGroup to be the
inverse of the new Reference's Condition so duplicates are avoided.
snakefoot
force-pushed
the
michailf-netstandard2.0_for_AtomicFile-tests
branch
from
aaacc86 to
f676cac
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs (1)
82-84: Optional: Enum values are GENERIC_, not FILE_GENERIC_.The values 0x80000000, 0x40000000, 0x20000000 are
GENERIC_READ,GENERIC_WRITE,GENERIC_EXECUTErespectively. The actualFILE_GENERIC_*constants are different (e.g.,FILE_GENERIC_READ= 0x00120089).Both work with
CreateFilesince generic access rights get mapped to specific rights, so this is functionally correct. Consider renaming toGENERIC_READ, etc., for accuracy, or adding comments clarifying this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs` around lines 82 - 84, The enum in NativeMethods.cs defines FILE_GENERIC_READ/WRITE/EXECUTE with values that are actually the GENERIC_* access masks; update the identifiers to GENERIC_READ, GENERIC_WRITE, GENERIC_EXECUTE (or add a clarifying comment) so the names accurately reflect the constants used; locate the enum entries FILE_GENERIC_READ, FILE_GENERIC_WRITE, FILE_GENERIC_EXECUTE in the NativeMethods class and either rename them to GENERIC_READ/GENERIC_WRITE/GENERIC_EXECUTE (adjust any references) or keep the names but add a comment stating these are generic access masks mapped by CreateFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NLog.Targets.AtomicFile/Internal/NativeMethods.cs`:
- Around line 82-84: The enum in NativeMethods.cs defines
FILE_GENERIC_READ/WRITE/EXECUTE with values that are actually the GENERIC_*
access masks; update the identifiers to GENERIC_READ, GENERIC_WRITE,
GENERIC_EXECUTE (or add a clarifying comment) so the names accurately reflect
the constants used; locate the enum entries FILE_GENERIC_READ,
FILE_GENERIC_WRITE, FILE_GENERIC_EXECUTE in the NativeMethods class and either
rename them to GENERIC_READ/GENERIC_WRITE/GENERIC_EXECUTE (adjust any
references) or keep the names but add a comment stating these are generic access
masks mapped by CreateFile.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/NLog.Targets.AtomicFile/AtomicFileTarget.cssrc/NLog.Targets.AtomicFile/Internal/NativeMethods.cs
snakefoot
force-pushed
the
michailf-netstandard2.0_for_AtomicFile-tests
branch
2 times, most recently
from
b51271a to
833a70d
Compare
|
|
|
Superseeded by #6100 |
2 participants
No description provided.