Skip to content

Conversation

@counter2015
Copy link
Contributor

@counter2015 counter2015 commented Nov 14, 2024

Fix: #328

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

First, I'd prefer to have separate pull requests for unrelated changes.

About the empty dir handling of zip files. I think it's a good idea to add an option/parameter to let the user decide whether to include directory entries or not. Enabling it could be the default, since it's what most users would expect.

@counter2015
Copy link
Contributor Author

@lefou okay, let me separate this pull request to two.

@counter2015
Copy link
Contributor Author

I'd like to do some "linting" works on #331 firstly.

@lihaoyi
Copy link
Member

lihaoyi commented Nov 21, 2024

Let's just do the zip file changes, no need all the english grammar improvements and other random build config tweaks.

@counter2015
Copy link
Contributor Author

Does format checking relies on OS system ?
In CI environment , I found

======================================================================================================================
1 tasks failed
mill.scalalib.scalafmt.ScalafmtModule.checkFormatAll Found 6 misformatted files
Error: Process completed with exit code 1.

In my local windows git bash environment

./mill -i mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources
[60/60] =============================================== mill.scalalib.scalafmt.ScalafmtModule/checkFormatAll __.sources =============================================== 21s
1 tasks failed
mill.scalalib.scalafmt.ScalafmtModule.checkFormatAll Found 253 misformatted files

one is 6 but another one is 253.

@counter2015
Copy link
Contributor Author

In CI Test

X test.os.ZipOpTests.unzipExcludePatterns 13ms 
[2205]   utest.AssertionError: paths == expected
[2205]   paths: IndexedSeq[os.Path] = ArraySeq(/Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/unzipAllExceptExcludingCertainFiles/File.txt, /Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/unzipAllExceptExcludingCertainFiles/folder1, /Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/unzipAllExceptExcludingCertainFiles/one.txt)
[2205]   expected: Seq[os.Path] = List(/Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/unzipAllExceptExcludingCertainFiles/File.txt, /Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/unzipAllExceptExcludingCertainFiles/one.txt, /Users/runner/work/os-lib/os-lib/out/mill-no-server/d4325d17/sandbox/out/scratch/ZipOpTests/tests/unzipExcludePatterns/folder1)

to simplify, paths is ArraySeq(File.txt, folder1, one.txt) while expected is Seq(File.txt, one.text, folder1), It seems that we should fix flaky test, sort the result before comparing ?

@lihaoyi
Copy link
Member

lihaoyi commented Nov 22, 2024

@counter2015 yes adding a sort seems reasonable

@counter2015 counter2015 marked this pull request as ready for review November 22, 2024 03:41
@counter2015
Copy link
Contributor Author

I found the test report depends on old commit, seems some weird caching issues in CI environment.

For example.

[1487] X test.os.CheckerTests.unzip 25ms 
[1487]   java.lang.AssertionError: assertion failed: ==> assertion failed: 3 != 2
[1487]     scala.Predef$.assert(Predef.scala:223)
[1487]     utest.asserts.Asserts$ArrowAssert.$eq$eq$greater(Asserts.scala:90)
[1487]     test.os.CheckerTests$.$anonfun$tests$99(CheckerTests.scala:481)
[1487]     test.os.CheckerTests$.$anonfun$tests$99$adapted(CheckerTests.scala:459)
[1487]     test.os.TestUtil$.$anonfun$prepChecker$2(TestUtil.scala:81)
[1487]     scala.util.DynamicVariable.withValue(DynamicVariable.scala:62)
[1487]     test.os.TestUtil$.$anonfun$prepChecker$1(TestUtil.scala:81)
[1487]     test.os.TestUtil$.prep(TestUtil.scala:77)
[1487]     test.os.TestUtil$.prepChecker(TestUtil.scala:81)
[1487]     test.os.CheckerTests$.$anonfun$tests$98(CheckerTests.scala:459)

for new test code

val unzipDir = os.unzip(
        source = zipFile,
        dest = wd / "unzipped"
      )
      os.walk(unzipDir).length ==> 3

Maybe we should re-run the CI command ?

@counter2015
Copy link
Contributor Author

@lihaoyi All test has passed excepted test on macos-latest, 11
image

It's stranged that the failed test cases seems not relative to this PR and might fails randomly.

@counter2015
Copy link
Contributor Author

Let me know if there's anything I can clarify or improve.

@lihaoyi lihaoyi merged commit 90626df into com-lihaoyi:main Dec 8, 2024
8 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Dec 8, 2024

Thanks @counter2015 , I tagged 0.11.4-M3 for you to try it out until we have another stable release

@lefou lefou added this to the 0.11.4 milestone Dec 8, 2024
joan38 added a commit to joan38/os-lib that referenced this pull request Jan 31, 2025
lihaoyi pushed a commit that referenced this pull request Jan 31, 2025
This reverts commit 90626df.
Fixes #349

I've spent some time trying to do a fix forward but I don't have much
time on my hand.
Can we revert this in the meantime and @counter2015 can provide another
PR for the empty folder issue?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

os.zip ignore empty directory.

4 participants