Skip to content

Add the concept of a top-level dependency #275

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 30, 2022
Merged

Add the concept of a top-level dependency #275

merged 1 commit into from
Sep 30, 2022

Conversation

vchuravy
Copy link
Member

Motivated by JuliaParallel/MPI.jl#646

In order for invalidation to happen we need to declare the dependency on the top-level of the file instead of in one of the platform specific wrappers.

@vchuravy vchuravy requested a review from giordano September 29, 2022 20:25
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

Can you please elaborate a bit on how you plan to use this concretely?

@vchuravy
Copy link
Member Author

Can you please elaborate a bit on how you plan to use this concretely?

For projects like OpenMPI_jll that use Preferences for artifact selection we need to add the package that holds the "key=>value" to the deps, thus we added OpenMPI_jll, but when the artifact is not available we create a cache file
in which we never have done using MPIPreferences. Thus the cache file is not invalidated when the cache file changes. Leading to the behaviour in JuliaParallel/MPI.jl#646

@giordano
Copy link
Member

OpenMPI_jll already depends on MPIPreferences in the project file: https://github.com/JuliaBinaryWrappers/OpenMPI_jll.jl/blob/a4b7bf25afd5e5c134e8e4f9117d47c2cdf12ac2/Project.toml#L6. I'm missing what this PR adds to that.

@vchuravy
Copy link
Member Author

When we cache a Package we do not care about the Project.toml, the cache file is only dependent on the code being parsed. We track what files are included and what Packages are pulled in with import/using. The cache file thus is missing a dependency edge on MPIPreferences and only MPIPreferences is invalidated when the Preference itself is changed.

@giordano
Copy link
Member

To understand: this means that we'll have using MPIPreferences in src/OpenMPI_jll.jl instead of src/wrappers/PLATFORM.jl?

@vchuravy
Copy link
Member Author

Yes exactly, we just move it up so that when there is no valid wrapper file we still add the dependency

@giordano
Copy link
Member

Ok, now I got it! 😃

@vchuravy
Copy link
Member Author

@giordano I don't think I am responsible for the 1.8 error xD

@giordano
Copy link
Member

# Fascinating, different versions of Julia have different opinions about GMP_jll in v1.7

🤦

@giordano
Copy link
Member

#276 should fix the failing test. I was probably too optimistic in thinking this could have been fixed for v1.8.2.

"""
struct Dependency <: AbstractDependency
pkg::PkgSpec
build_version::Union{VersionNumber,Nothing}
compat::String # semver string for use in Project.toml of the JLL
platforms::Vector{<:AbstractPlatform}
top_level::Bool
Copy link
Member

Choose a reason for hiding this comment

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

It just occurred to me that top-level dependencies can only be RuntimeDependency, right? Or do you see cases where it can be a built-time dependency too?

Copy link
Member

Choose a reason for hiding this comment

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

This would make #278 largely unnecessary because those dependencies would be pulled in during the build, and so they wouldn't cause the extra warning in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Uhm the only case is @maleadt current adventure where the availability of a artifact might change the platform selection of the current one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a more principled stance would be to.make everything that is AnyPlatform top level?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more principled stance would be to.make everything that is AnyPlatform top level?

Uhm, no iso_codes_jll is a regular JLL which is AnyPlatform, I'm not sure that should be a top-level dependency.

Copy link
Member

Choose a reason for hiding this comment

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

Also, AnyPlatform() packages are often build-time dependencies (e.g. header-only packages), iso_codes_jll is an exception but we have to deal with that.

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