Skip to content

Move HTTP and XRootD related stuff into extensions#396

Merged
tamasgal merged 5 commits into
mainfrom
less-deps
Dec 5, 2025
Merged

Move HTTP and XRootD related stuff into extensions#396
tamasgal merged 5 commits into
mainfrom
less-deps

Conversation

@tamasgal

@tamasgal tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member

The only part which I am stuck at right now is in src/root.jl:

function ROOTFile(filename::AbstractString; customstructs = Dict("TLorentzVector" => LorentzVector{Float64}))
    fobj = if startswith(filename, r"https?://")
        HTTPStream(filename)
    elseif startswith(filename, "root://")
        XRDStream(filename)
    else
        !isfile(filename) && throw(SystemError("opening file $filename", 2))
        MmapStream(filename)
    end

since both HTTPStream and XRDStream are defined in extensions, which causes a problem of course.

The question is then: how to deal with a type-matching based on string value with potentially undefined types.

For now I went with a symbol-based dynamic approach (which fails, but I will have a look asap):

    fobj = if startswith(filename, r"https?://")
        typesymbol = :HTTPStream
        !isdefined(UnROOT, typesymbol) && error("Opening HTTP streamed ROOT files requires to install and load the 'HTTP' module.")
        getproperty(UnROOT, typesymbol)(filename)
    elseif startswith(filename, "root://")
        typesymbol = :XRDStream
        !isdefined(UnROOT, typesymbol) && error("Opening XRootD streamed ROOT files requires to install and load the 'XRootD' module.")
        getproperty(UnROOT, typesymbol)(filename)
    else
        !isfile(filename) && throw(SystemError("opening file $filename", 2))
        MmapStream(filename)
    end

@Moelf

Moelf commented Dec 5, 2025

Copy link
Copy Markdown
Member

I think you can define an empty

function HTTPStream end

in the base package, and extend it in the extension package https://github.com/JuliaHEP/Minuit2.jl/blob/9b2eea06693853fb720118a64d8de16b13dd80a0/ext/Minuit2PlotsExt.jl#L15

@codecov

codecov Bot commented Dec 5, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 64.04494% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.32%. Comparing base (c05d324) to head (6a0c0e4).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
ext/UnROOTXRootDExt.jl 46.66% 16 Missing ⚠️
ext/UnROOTHTTPExt.jl 80.43% 9 Missing ⚠️
src/streamsource.jl 36.36% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #396      +/-   ##
==========================================
- Coverage   84.41%   84.32%   -0.09%     
==========================================
  Files          21       23       +2     
  Lines        3099     3107       +8     
==========================================
+ Hits         2616     2620       +4     
- Misses        483      487       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tamasgal

tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member Author

I yeah, that's probably the easiest

@tamasgal

tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member Author

Hmm, that will not work:

ERROR: LoadError: cannot assign a value to imported variable UnROOTHTTPExt.HTTPStream

@tamasgal

tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member Author

Ah OK, I know what to do. I will convert that to actual function calls...

@tamasgal tamasgal marked this pull request as ready for review December 5, 2025 15:57
@tamasgal

tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member Author

OK, this will work. Ready @Moelf

We could then do 0.11 because it's breaking.

@tamasgal

tamasgal commented Dec 5, 2025

Copy link
Copy Markdown
Member Author

I will also add a README entry for the breaking change ;)

@tamasgal tamasgal merged commit 203a3e2 into main Dec 5, 2025
8 of 11 checks passed
@tamasgal tamasgal deleted the less-deps branch December 5, 2025 19:59
@tamasgal tamasgal mentioned this pull request Dec 5, 2025
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.

2 participants