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

[infrastructure] auto-create addons-BOM #5551

Merged
merged 4 commits into from
May 9, 2019
Merged

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented May 4, 2019

This allows to auto-create the addons-BOM from the bundles-POM.

The problem is that it needs an extra maven-call mvn antrun:run@create-bom in front of mvn clean install because the bom-POM is otherwise parsed before it is created. Therefore I left it in place.

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

@J-N-K J-N-K added the infrastructure Build system and Karaf related issues and PRs label May 4, 2019
@J-N-K J-N-K requested a review from a team as a code owner May 4, 2019 10:29
@davidgraeff
Copy link
Member

Fixes #5214

</dependency>
<dependency>
<groupId>org.openhab.addons.bundles</groupId>
<artifactId>org.openhab.binding.dlinksmarthome</artifactId>
<version>${project.version}</version>
<version>2.5.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Could we also use the project version some way? It might make releasing harder? What is the plan should this file stay in the repo and be comitted after running the goal manual (like the copyright headers?)

Or is there some other plan?

Copy link
Member Author

@J-N-K J-N-K May 4, 2019

Choose a reason for hiding this comment

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

version: Not so easy, I'll try. The problem is that maven replaces the place-holder in l. 72 of the pom before replacing the file-content. I don't know how to prevent that.

I would prefer to run it on each clean install (i.e. re-build it every time) and remove it from the repo. But maven will not start at all if the bom-pom is missing.

Copy link
Member

Choose a reason for hiding this comment

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

Which is strange, because the BOM is not used within this repo and is really just a generated file to be uploaded to bintray.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/openhab/openhab2-addons/blob/169cd570b397d057b450bb86bc5ac50446e94bf7/pom.xml#L768

Is the culprit. This includes the bom/pom.xml which includes the openhab-addons/pom.xml. If we remove it from the repo this is missing and maven complains.

</dependency>
<dependency>
<groupId>org.openhab.addons.bundles</groupId>
<artifactId>org.openhab.binding.mqtt.homeassistant</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

@davidgraeff is this all expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some bundles where in another ordering (maybe mistake) and some where missing at all. I don't think that was by intention.

Copy link
Member

Choose a reason for hiding this comment

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

not by intention, correct. The BOM is not yet used anywhere afaik, so nobody noticed.

bom/pom.xml Outdated
<replacevalue><![CDATA[<parent>
<groupId>org.openhab.addons.bom</groupId>
<artifactId>org.openhab.addons.reactor.bom</artifactId>
<version>2.5.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this dynamic this will make releasing though

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be replaced by the project.version, true.

Copy link
Member

Choose a reason for hiding this comment

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

It's the parent reference so not trivial though

Copy link
Member Author

Choose a reason for hiding this comment

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

since maven replaces it with the value that is valid during the run, it's easy.

<goals>
<goal>run</goal>
</goals>
<configuration>
Copy link
Member

Choose a reason for hiding this comment

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

Can we link some phase here to put in the right location of the build cycle or is this a deliberate choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we can't run that during our normal build (see above). If we have to execute it manually as seperate task anyway, we don't need a phase. I could add it to generate-sources, if you think that is useful.

@martinvw
Copy link
Member

martinvw commented May 4, 2019

Thanks @J-N-K

@davidgraeff
Copy link
Member

davidgraeff commented May 4, 2019

@maggu2810 I think the checked in openhab2-addons bom file is used for bnd to generate the bnd index-something-something repositories? Would it be possible to remove the checked in variant and auto-generate it like in this pr for bnd?

@kaikreuzer
Copy link
Member

Please make sure that when removing any files and generate them instead the IDE still works - we should not require a full Maven build before the IDE is usable for add-on development.

@maggu2810
Copy link

I don't understand why the POM should be removed.
Do I need to generate it all the time first to have a working setup?

If it is just for tooling to update the existing one similar to update the license in front of check in it is another story but as you used "remove"...

If it is to update an existing one IMHO you should replace the hard-coded version by {project.version} as it is done already.
The checked in BOM should refer to the version of the whole projects and not to a specific one.

Why making the life harder again?

@maggu2810
Copy link

  • Currently you can checkout the repo and call org.codehaus.mojo:versions-maven-plugin:2.7:set -DnewVersion=... to set the version of the whole reactor. This will be broken is an hardcoded version is used in the BOM.

  • If the regeneration of the BOM in front of a build is necessary, it is IMHO really bad. I would expect to checkout a Maven repo and call mvn clean install to get all build of the repo without the requirement to execute other commands to get stuff of the repo generated first.

  • If I use an IDE with the UI repo and the addons repo and the demo setup, the demo setup will reference to the BOMs. I would like to add new addons to the addons BOM and new UIs to the UI BOM and the demo should reflect that changed. I don't want to need to call some new maven goals manually first.

@davidgraeff
Copy link
Member

That's why I'm asking for the use-case. At the moment it seems for me that the bom is only used for being uploaded like a build artifact and in that case it can be generated.

@maggu2810
Copy link

maggu2810 commented May 4, 2019

It is also used by the Bnd resolver (not only the explicit called Maven goal, but also in the IDE implicit by the Bndtools).

As soon as it is used, it should also be present.
It is common practice (at least for the Eclipse IDE) that loaded projects are preferred in front of stuff from the local Maven repo.
Otherwise you would need to call "mvn install" all the time after you did a change on a source file.

@davidgraeff
Copy link
Member

I suspected that. In contrast to openhab core this repo contains independent bundles, so it makes sense to keep as much bundle specific information (karaf feature definition, maven dependencies etc) in the corresponding bundle directory.

Makes it easier for maintainers to review and for new binding PRs to run only a subset of the repo.
If we can't solve the bnd resolving issue in another way than via a repo BOM (but I don't really get why tbh), then at least this auto-generation feature helps to keep the BOM file correct.

@maggu2810
Copy link

I assume people that would like to develop a new bundle needs to read at least some lines of documentation.
As soon as they would like to test their new bundle in the IDE they will follow the instructions and add the dependency in the BOM POM file to use it in the Demo application.
So I don't think it will be forgotten such often.

You would also like to move the Karaf feature definition to the correspondent bundle directory?
After that you would like to create another feature file that aggregates all the other ones?
Or should the distribution import all the single Karaf feature repositories?

@davidgraeff
Copy link
Member

davidgraeff commented May 4, 2019

See https://github.com/openhab/openhab2-addons/issues/5222 and the corresponding PR. The file is merged in the current PR for uploading. The single feature files in each bundle directory are standalone valid though.

My goal at the moment is to get travis ci running with full test and checks and feature verification enabled for new binding PRs (#5389). But I cannot check the BOM in a sane way. (Until now: Where I can auto generate and compare it).

I assume people that would like to develop a new bundle needs to read at least some lines of documentation.

That would be wonderful, but you'd be surprised. Not much is read. And I like travis to fail if such basic tasks like the BOM amending are not done. (Or not having the static BOM at all, that's why I asked).

@maggu2810
Copy link

I assume I get your point to move all bundle specific stuff below one bundle specific directory and all that stuff can be tested on its own.

  • This will also involve the integration tests that needs to be moved again.
  • The BOM of the repo needs to be generated by the coordinated of the bundle.
  • The feature file of the repo needs to aggregate the bundle specific feature.

If the openHAB Addons maintainer would like to use that approach, IMHO:

  • there should be a maven goal that generates a feature template but after that the feature file should be used and can be modified manually (so, similar to an archetype)
  • the feature file AND the BOM file should be regenerated in front of the checkin and part of the repository similar as you should generate / update the license headers in front of the checkin etc.

@J-N-K
Copy link
Member Author

J-N-K commented May 5, 2019

For the feature: we should discuss that in #5555.

For here I assume that using it just as a tool for manual re-creation is ok. I'll check what I can do about the {project.version}. My problem here is that maven replaces it in my ant-directives.

J-N-K added 3 commits May 5, 2019 19:18
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@J-N-K
Copy link
Member Author

J-N-K commented May 5, 2019

Fixed the issue with the version, looks a bit hackish but probably the best way we can do.

bom/pom.xml Outdated Show resolved Hide resolved
bom/openhab-addons/pom.xml Outdated Show resolved Hide resolved
Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@davidgraeff
Copy link
Member

Nobody seems to veto, so I'm going to merge and amend the documentation by that command.

@davidgraeff davidgraeff merged commit bb87c4b into openhab:master May 9, 2019
@J-N-K J-N-K deleted the auto-bom branch May 9, 2019 19:39
mherwege pushed a commit to mherwege/openhab-addons that referenced this pull request May 10, 2019
* auto-create addons-BOM

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
@wborn wborn added this to the 2.5 milestone May 26, 2019
Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Jun 19, 2019
* auto-create addons-BOM

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Pshatsillo <pshatsillo@gmail.com>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
* auto-create addons-BOM

Signed-off-by: Jan N. Klug <jan.n.klug@rub.de>
Signed-off-by: Maximilian Hess <mail@ne0h.de>
@wborn wborn removed this from the 2.5 milestone Dec 8, 2019
@wborn wborn added this to the 2.5 milestone Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Build system and Karaf related issues and PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants