Skip to content

fix: preserve Zenodo archive extraction layout - #19

Open
lfvasconcelos wants to merge 1 commit into
ViennaInstitute:masterfrom
lfvasconcelos:fix/zenodo-zip-layout
Open

lfvasconcelos wants to merge 1 commit into
ViennaInstitute:masterfrom
lfvasconcelos:fix/zenodo-zip-layout

Conversation

@lfvasconcelos

Copy link
Copy Markdown

Summary

  • extract the Zenodo archive relative to the working directory, matching the former unzip behavior
  • prevent duplication of the archive's existing data/010_eurostat_tables/ prefix
  • add a cross-platform regression test that builds a representative ZIP fixture and checks the extracted path

Context

PR #12 identified that the system unzip command was not portable to Windows. The alternative crossplattform-zip branch fixed the portability issue with p7zip_jll, and commit ebfb7f4 was subsequently tested successfully on Windows.

master currently contains the earlier commit 704c5db, which passes -odata/010_eurostat_tables to 7-Zip. Because the Zenodo archive already contains that directory prefix, extraction produces data/010_eurostat_tables/data/010_eurostat_tables/... and downstream readers cannot find the files.

This change applies the extraction behavior from ebfb7f4 and adds a regression test for the actual archive layout.

Validation

  • Windows 11, Julia 1.11.5
  • CalibrateBeforeIT package load and precompilation: passed
  • test/test_download_function.jl: 16/16 tests passed, including the regression fixture containing data/010_eurostat_tables/payload.txt
  • git diff --check upstream/master...HEAD: passed

Extract relative to the working directory so archives that already contain the data/010_eurostat_tables prefix do not create a duplicated path. Add a regression test for that layout.

Co-authored-by: Oliver Reiter <reiter@wiiw.ac.at>
@lfvasconcelos

Copy link
Copy Markdown
Author

Rebased onto current master (1f302d2). New head is 237cabc; the diff is unchanged, only the base moved. @zauster, the run for it is waiting on "Approve and run workflows" whenever you have a moment.

Why the earlier work did not fully close this. The crossplattform-zip branch carried two relevant commits, but only the first one, 704c5db, reached master through merge 06aeb78. PR #18 was closed rather than merged, so the follow-up ebfb7f4 never landed.

704c5db did fix the portability problem: the system unzip call is gone and p7zip_jll works on Windows. But it also introduced extract_to = "data/010_eurostat_tables" and passes it to 7-Zip as -o$(extract_to). The Zenodo archive already carries that prefix internally, so extraction now yields:

data/010_eurostat_tables/data/010_eurostat_tables/...

Every downstream reader looks under eurostat_path = "data/010_eurostat_tables" (src/CalibrateBeforeIT.jl:86, src/import_eurostat.jl:379, 02_preprocess_raw_eurostat_tables.jl:14, among others), so on current master the files sit one level deeper than anything expects. The old unzip -o $zip_file call had no output target and extracted relative to the working directory, which is why the layout was right before.

This PR restores that behavior and adds a regression test that builds a fixture archive with the real prefix and asserts the extracted path.

Two things worth flagging for review:

  • I took only the download_zenodo.jl behavior from ebfb7f4, not its Project.toml change. That commit also dropped the p7zip_jll compat entry, which 238ccb5 later pinned to "17". This PR leaves Project.toml alone.
  • The extract_to positional argument and the function's return value are removed, so this is a small API change. No caller in the repo uses either; 01a_download_zenodo_tables.jl calls the function with no arguments. If you would rather not touch the signature, I can keep extract_to as an optional argument defaulting to the working directory.

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.

1 participant