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

Incorrect changes after running update-extension-dependencies.sh #22836

Closed
tsegismont opened this issue Jan 12, 2022 · 13 comments · Fixed by #22993
Closed

Incorrect changes after running update-extension-dependencies.sh #22836

tsegismont opened this issue Jan 12, 2022 · 13 comments · Fixed by #22993
Assignees
Labels
area/build kind/bug Something isn't working
Milestone

Comments

@tsegismont
Copy link
Contributor

If you:

Then the output contains references to the modules that were moved out of the repository.

This was found while working on #22810

@famod
Copy link
Member

famod commented Jan 12, 2022

The cause is described here: #22810 (comment)

@aloubyansky do you want to take this?

@aloubyansky
Copy link
Member

I am busy with other tasks currently, so not right now.

@famod
Copy link
Member

famod commented Jan 12, 2022

I'll try to have look at using that relocations profile later today.

@gsmet
Copy link
Member

gsmet commented Jan 12, 2022

I'll try to have look at using that relocations profile later today.

I'm not sure it's the right fix. You can use it in the script but personally, I build things separately to make sure it builds before running the script.

And we don't want to enable this profile always as one of the point of this work was to reduce the number of modules.

I think we can probably live with this limitation (but I would fix it in the build you have in the script, so we could say "just ask for a full build when the script asks to fix it"), especially since CI complains loudly.

@famod
Copy link
Member

famod commented Jan 12, 2022

To be clear: the script only builds on top of what's in that generated descriptor and that generation picks up "removed" extensions.
So I'd prefer if we could fix the root cause of this.

@famod
Copy link
Member

famod commented Jan 16, 2022

Good news: -Prelocations seems to fix it.

I don't have time to check whether we can fix the root cause in the descriptor generation, so we have two options AFAICS:

  • -Prelocations the default
  • or, in the script, don't ask the user if the entire projects is already built and always build with -Prelocations

I suppose we want to go with option two?

@famod
Copy link
Member

famod commented Jan 16, 2022

I found one difference with -Prelocations when compared with the result you get with a clean local repo and build without -Prelocations:

     {
    "name" : "Amazon Services Common",
    "description" : "Common components for AWS Lambda functions",
    "metadata" : {
      "keywords" : [ "amazon services", "aws", "amazon" ],
      "categories" : [ "data" ],
      "status" : "stable",
      "unlisted" : true,
      "built-with-quarkus-core" : "999-SNAPSHOT",
      "extension-dependencies" : [ "io.quarkus:quarkus-core", "io.quarkus:quarkus-arc", "io.quarkus:quarkus-netty" ]
    },
    "artifact" : "io.quarkus:quarkus-amazon-common::jar:999-SNAPSHOT",
    "origins" : [ "io.quarkus:quarkus-bom-quarkus-platform-descriptor:999-SNAPSHOT:json:999-SNAPSHOT" ]
  }

This exists after building with -Prelocations with those old extensions still in local repo.

@famod
Copy link
Member

famod commented Jan 16, 2022

It seems we're missing a relocation for quarkus-amazon-common. I'll add that to my PR.

@gsmet
Copy link
Member

gsmet commented Jan 16, 2022

@famod it's not missing. I don't think we need one given it's not a user-exposed extension.

Let's not make things just to fix a totally unrelated issue.

@famod
Copy link
Member

famod commented Jan 16, 2022

@gsmet It's not unrelated. If -Prelocations still adds it to the descriptor, then the script will add it to the pom.xml and we again have a deviation.

@famod
Copy link
Member

famod commented Jan 16, 2022

The alternative would be to filter out that one artifact in the script (which I find a bit brittle, but it's doable).
This would not prevent it from ending up in the descriptor, though, which means that locally even with -Prelocations you'd need to purge your local repo to have a consistent descriptor. (One could argue that this will be needed anyway when we start to remove relocations again...)

I'll wait until you @gsmet and @aloubyansky have decided which way to go.

@gsmet
Copy link
Member

gsmet commented Jan 16, 2022

It's unrelated because you're trying to overcome an issue with a script by adding some stuff that shouldn't be related. You basically need relocations to say "this thing is not an extension anymore". That shouldn't be necessary.

What I fail to see is why we are somehow getting all the extensions that are in the local Maven repo? Why isn't it coming from what's in the tree or are at least filtered with what's in the tree.

@famod
Copy link
Member

famod commented Jan 16, 2022

What I fail to see is why we are somehow getting all the extensions that are in the local Maven repo? Why isn't it coming from what's in the tree or are at least filtered with what's in the tree.

That's what I meant with

So I'd prefer if we could fix the root cause of this.

which has nothing to do with the script in the first place. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants