Skip to content
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

Add parent packages into dependency tree #539

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kubajj
Copy link
Contributor

@kubajj kubajj commented Aug 10, 2021

This PR adds an array of indices of parent nodes to each node of the dependency tree. It is part of my Handling compiler arguments GSoC 2021 project and was separated from #498 for easier review.

Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Thanks, I left a few comments.

I'm not sure whether the parent list should actually be optional in the cache. Without requiring the parent list an existing cache could be loaded and might not contain this information. Instead we should require the data and invalidate the cache if the parent list is not present.

Also, we should add a unit test to make sure the parent list is written and read correctly from the cache.

src/fpm/dependency.f90 Outdated Show resolved Hide resolved
src/fpm/dependency.f90 Outdated Show resolved Hide resolved
src/fpm/dependency.f90 Outdated Show resolved Hide resolved
@kubajj
Copy link
Contributor Author

kubajj commented Aug 10, 2021

I'm not sure whether the parent list should actually be optional in the cache.

I think that as main package does not have any parents, wouldn't this cause an issue?

Without requiring the parent list an existing cache could be loaded and might not contain this information. Instead we should require the data and invalidate the cache if the parent list is not present.

Should I call it with requested=.true. or just check the stat and raise an error if it is not toml_stat%success?

@awvwgk
Copy link
Member

awvwgk commented Aug 10, 2021

I think that as main package does not have any parents, wouldn't this cause an issue?

The list would be empty in this case. For a cyclic dependency the main project might as well have a parent.

Should I call it with requested=.true. or just check the stat and raise an error if it is not toml_stat%success?

This is more difficult, in principle we could just request it but this potentially result in no connections in case of an old cache. Therefore, best is to not request it and create an error if it is not associated. This should automatically invalidate the cache and fpm will recreate it by building the dependency tree again.

@kubajj
Copy link
Contributor Author

kubajj commented Aug 11, 2021

Should I call it with requested=.true. or just check the stat and raise an error if it is not toml_stat%success?

This is more difficult, in principle we could just request it but this potentially result in no connections in case of an old cache. Therefore, best is to not request it and create an error if it is not associated. This should automatically invalidate the cache and fpm will recreate it by building the dependency tree again.

Should this really be an error? I thought that we should just dump the cache and build the dependency tree on our own if the parent table is missing.

@awvwgk
Copy link
Member

awvwgk commented Aug 11, 2021

Generating an error while reading the cache is okay. An error signals an invalid cache file and the caller is responsible for handling the error by invalidating the cache file (i.e. deleting it).

@kubajj
Copy link
Contributor Author

kubajj commented Aug 11, 2021

Generating an error while reading the cache is okay. An error signals an invalid cache file and the caller is responsible for handling the error by invalidating the cache file (i.e. deleting it).

So I should modify the add_project subroutine to delete cache if self%load fails?

@awvwgk
Copy link
Member

awvwgk commented Aug 11, 2021

This should already be the default behaviour:

fpm/src/fpm/dependency.f90

Lines 237 to 240 in d72d953

if (allocated(self%cache)) then
call self%load(self%cache, error)
if (allocated(error)) return
end if

Right this is not done yet. add_project would be the right place to fix this.

@kubajj
Copy link
Contributor Author

kubajj commented Aug 11, 2021

This should already be the default behaviour:

fpm/src/fpm/dependency.f90

Lines 237 to 240 in d72d953

if (allocated(self%cache)) then
call self%load(self%cache, error)
if (allocated(error)) return
end if

Right this is not done yet. add_project would be the right place to fix this.

Can I create an empty toml array? I am struggling to pass ci tests for example packages with just the main package.

@awvwgk
Copy link
Member

awvwgk commented Aug 11, 2021

After looking into this for a while I wonder why have to save the parents of each dependency. Saving the dependees of each dependency is unpractical as adding a dependency requires to potentially modify all nodes. Instead saving the dependencies of each dependency, i.e. the child nodes, keeps the information about a dependency local in each node.

In the context of applying profiles we should be able to walk the tree from the main project while applying either the parent profile or overwriting it with a profile from the current project.

cc @LKedward @everythingfunctional

@LKedward
Copy link
Member

Apologies for the late notice Sebastian @awvwgk, but are you available tomorrow at 3PM UTC to join our mentor chat? It might be quicker to discuss this altogether.

@awvwgk
Copy link
Member

awvwgk commented Aug 11, 2021

Sure, should be able to join, I'll make a note for tomorrow 3PM UTC/5PM CEST.

@awvwgk
Copy link
Member

awvwgk commented Aug 12, 2021

As discussed I will take over the implementation of the retaining the topology of the dependency tree and implementing a mechanism to transverse the tree later when building up the model (see #355 for more details).

@awvwgk awvwgk self-assigned this Aug 12, 2021
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.

3 participants