Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix Zip64ExtraField handling - #111802

Merged
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes
Jan 24, 2025
Merged

Fix Zip64ExtraField handling#111802
carlossanlop merged 2 commits into
dotnet:mainfrom
edwardneal:zip64-block-writes

Conversation

@edwardneal

Copy link
Copy Markdown
Contributor

Bugfix for the outer loop tests which failed after PR #103153.

The original Zip64ExtraField.WriteBlock method contained logic to conditionally write the compressed size, uncompressed size, local header offset and starting disk number to the output stream. When this was modified to use BinaryPrimitives, (link) this condition wasn't ported across properly - it conditionally wrote into the destination buffer, but used constant field offsets. There was also one error in ZipArchiveEntry.WriteDataDescriptor - that's now corrected.

I don't see this pattern repeated anywhere else in ZipBlocks or ZipArchiveEntry. ZipArchiveEntry does sometimes write a ZIP64 data descriptor (link) but this logic is reasonable.

Outer loop tests which failed as a result of this bug:

  • System.IO.Compression.Tests.zip_LargeFiles.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
  • System.IO.Compression.Tests.zip_LargeFiles.UnzipOver4GBZipFile
  • System.IO.Packaging.Tests.LargeFilesTests.CheckZIP64VersionIsSet_ForSmallFilesAfterBigFiles
    These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Apologies again for missing this.

@carlossanlop - if you're happy with the diff, could you run the outer loop tests against this PR again please?

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 24, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp list

@azure-pipelines

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@carlossanlop

Copy link
Copy Markdown
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlopcarlossanlop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the quick fix submission. LGTM. Let's see what outerloop says.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • These were the only tests which failed because they're the only ones which generated large enough files to trigger the logic which wrote Zip64ExtraField. They're now passing locally.

Just double checking: The outerloop tests get skipped by default AFAIK even when you run them locally, unless you pass the necessary msbuild global property in the command (if from CLI). Did you do this by any chance?

I think such suppression does not happen if you run the unit test from VS or VS Code instead. You can choose the specific test and it gets executed.

@edwardneal

Copy link
Copy Markdown
ContributorAuthor

The VS test runner skips outerloop tests, even if I specifically select them. Those tests remain in the "not executed" state, but when that state rolls up to the parent, it looks like the entire suite passed...

It's an odd problem, but I've brute-forced it for the moment - I just locally commented out the OuterLoop attributes and ran the entire test suite for System.IO.Compression and System.IO.Packaging, specifically confirming that the previously-failing tests pass.

@carlossanlop

Copy link
Copy Markdown
Contributor

Thanks for checking! So far the results don't show compression-related failures. I'm monitoring the remaining ones too.

@carlossanlop

Copy link
Copy Markdown
Contributor
  • runtime (Build browser-wasm linux Release LibraryTests) was cancelled already, but showing up as stuck.
  • I opened issues for all the unrelated outerloop failures. Didn't find any Zip related errors.

Merging now!

grendello added a commit to grendello/runtime that referenced this pull request Jan 27, 2025
* main: (22 commits)
Clean up Stopwatch a bit (dotnet#111834)
JIT: Fix embedded broadcast simd size (dotnet#111638)
Revert potential UB due to aliasing + more WB removals (dotnet#111733)
re-enable acceleration of Vector512<long>.op_Multiply (dotnet#111832)
Handle OSSL 3.4 change to SAN:othername formatting
JIT: Fix stack allocated arrays for NativeAOT (dotnet#111827)
JIT: enhance RBO inference for similar compares to constants (dotnet#111766)
JIT: Don't run optSetBlockWeights when we have PGO data (dotnet#111764)
[Android] Make sure RuntimeFlavor=CoreCLR when clr subset is specified (dotnet#111821)
Change empty subject test certificate to include a critical SAN.
Fix reversed code offsets in GcInfo (dotnet#111792)
Swap some libraries areas between leads (dotnet#111816)
Add left-handed spherical and cylindrical billboards (dotnet#109605)
JIT: revise `optRelopImpliesRelop` to always set `reverseSense` (dotnet#111803)
Fix Zip64ExtraField handling (dotnet#111802)
Add build support for Android+CoreCLR (dotnet#110471)
arm64: Add bic(s) compact encoding (dotnet#111452)
JIT: Ensure `BBF_PROF_WEIGHT` flag is set when we have PGO data (dotnet#111780)
Add support for AVX10.2, Add AVX10.2 API surface and template tests (dotnet#111209)
JIT: Preliminary for enabling inlining late devirted calls (dotnet#111782)
...
@carlossanlopcarlossanlop mentioned this pull request Feb 13, 2025
15 tasks
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Feb 24, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IO.Compressioncommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@edwardneal@carlossanlop