Unify build workflows and argument specification

[HannesWell]

This PR unifies the build in the following points

• Merge the external-pr workflow into the maven workflow
• Create .mvn/maven.config and common build arguments to it
• Remove unused environment variable 'WORKSPACE'

Please consider this as suggestion and let me know if I oversaw a usage of the removed elements.

[HannesWell]

Additionally this PR suggests a way to make the additional external-pr workflow obsolete.
It simply skips the execution of the additional jobs if you open a PR from a branch in this repo (which I assume is what you wanted to achieve), but without the need to maintain the same workflow twice.

You can see the result here: HannesWell#2

This is somehow an enhanced version of what was suggested in #2211 (comment).

[HannesWell]

OK, I now noticed that the build-maven-artifacts job has a different root/working directory. Restored (and simplified) that part.

[LorenzoBettini]

@HannesWell Thank you for restoring the Maven build. I was about to tell you about that: it's better to keep it because, in the past, we had troubles with the maven plugins depending on the full target platform in the full build.

Cool that you found a way to deal with external PRs better! Are you sure it works as expected? I mean, you tested it internally by creating a PR against your repo, right? But what happens if the PR is really external?

Concerning -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn. That has always been in the Jenkins jobs, but for GitHub Actions, I'd like to see the log of Maven dependency resolution: in the past, the log helped me a lot to track down dependency problems.

"Glad" the macOS build fails for you as well: I started to see that problem in my fork as well, and I guess that's due to Maven 3.9.1 used in the macOS environment and maybe to some dependency problems of mwe2 (but I'll open another issue for that).

[HannesWell]

@HannesWell Thank you for restoring the Maven build. I was about to tell you about that: it's better to keep it because, in the past, we had troubles with the maven plugins depending on the full target platform in the full build.

Understand 👍🏽

Cool that you found a way to deal with external PRs better! Are you sure it works as expected? I mean, you tested it internally by creating a PR against your repo, right? But what happens if the PR is really external?

I'll test that case with a second fork and let you know, but I'm optimistic.

Concerning -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn. That has always been in the Jenkins jobs, but for GitHub Actions, I'd like to see the log of Maven dependency resolution: in the past, the log helped me a lot to track down dependency problems.

Understand, I restored that.

"Glad" the macOS build fails for you as well: I started to see that problem in my fork as well, and I guess that's due to Maven 3.9.1 used in the macOS environment and maybe to some dependency problems of mwe2 (but I'll open another issue for that).

That could be possible. As far as I know Tycho 2.7.5 is not for sure compatible with Maven 3.9.1 and the GH-runners seems to have different maven versions. Some have 3.8.6 and some 3.9.1

[HannesWell]

Furthermore I inlined the value of the JENKINS_URL variable, because I assume it is very unlikely that one uses another URL for that value. Or is that a common use-case?

Furthermore I would like to suggest to inline the scripts used in the Jenkins piplines. That would make the pipelines simpler to understand since there is less indirection.
If they are also intended to be run by developers I suggest to use a corresponding M2E Maven launch configuration instead. This has the advantage that it works on all platforms and not only on Linux (and mac?) or if one has cygwin installed.

In general there are many scripts in the repo root and multiple seem to serve the same purpose and I wonder if they are still used. If they are only used in the Jenkinsfile I suggest to inline them and reconsider if they are used locally. For example I would assume that signing is only performed in Jenkins and therefore is no need to specify a separate script for that.

[cdietrich]

the jenkins url stems from times where our main builds were still done on typefox infrastructure

[LorenzoBettini]

Furthermore, IIRC, Maven 4 will prohibit properties in repositories. For me, it's OK to remove the JENKINS_URL.

Concerning inlining the scripts, I think I was told to leave them there. I don't have strong opinions on that. We don't even use them in GitHub Actions anyway. I leave the decision to @cdietrich .

[LorenzoBettini]

@HannesWell I'm fine with PR, apart from the decision on script files as I said above.

Moreover, could you please confirm that PRs work as intended? I mean, you verified that it does not start in the same repository, but what about if it's really external? I'm not sure the pre-merge of PRs in GitHub Actions takes into consideration your own changes to the YAML files. Or does it?

[HannesWell]

Furthermore, IIRC, Maven 4 will prohibit properties in repositories. For me, it's OK to remove the JENKINS_URL.

I don't fully understand what you mean by that? Are you referring to the plan that with maven-4 the consumer pom will only contain the resolved values of properties used inside the pom?

Concerning inlining the scripts, I think I was told to leave them there. I don't have strong opinions on that. We don't even use them in GitHub Actions anyway. I leave the decision to @cdietrich .

Understand, then @cdietrich can you please decide?

@HannesWell I'm fine with PR, apart from the decision on script files as I said above.

OK, great. I just removed of few more occurances of the JENKINS_URL.

Moreover, could you please confirm that PRs work as intended? I mean, you verified that it does not start in the same repository, but what about if it's really external?

I would like to do that, but unfortunately I cannot create a second fork. I tried to pretend an external PR by temporarily changing the base URL in HannesWell#3, but this of course still receives the push events and therefore runs all jobs twice. But it at least shows that the pull_request event based builds are then executed. Therefore I'm confident that this will work as expected. But if you want to be really sure, just please open a PR against my fork. I pushed the head of this PR to the main branch of my fork. So you could just push any change.

I'm not sure the pre-merge of PRs in GitHub Actions takes into consideration your own changes to the YAML files. Or does it?

From my observations and as far as I know in PRs only the changes from people with write access to the repository are taken into account immediately. The same applies for the Jenkins pipeline. Otherwise external people could simply extract credentials by modifying a workflow/Jenkinsfile and open a PR. But since I'm not a committer in xText my changes are not taken into account immediately.

[LorenzoBettini]

Furthermore, IIRC, Maven 4 will prohibit properties in repositories. For me, it's OK to remove the JENKINS_URL.

I don't fully understand what you mean by that? Are you referring to the plan that with maven-4 the consumer pom will only contain the resolved values of properties used inside the pom?

I mean that, IIRC, in Maven 4.0.0, you cannot use a property when specifying a URL of a <repository> or <pluginRepository>: they must be constants and fixed.

I would like to do that, but unfortunately I cannot create a second fork. I tried to pretend an external PR by temporarily changing the base URL in HannesWell#3, but this of course still receives the push events and therefore runs all jobs twice. But it at least shows that the pull_request event based builds are then executed. Therefore I'm confident that this will work as expected. But if you want to be really sure, just please open a PR against my fork. I pushed the head of this PR to the main branch of my fork. So you could just push any change.

I'm not sure the pre-merge of PRs in GitHub Actions takes into consideration your own changes to the YAML files. Or does it?

From my observations and as far as I know in PRs only the changes from people with write access to the repository are taken into account immediately. The same applies for the Jenkins pipeline. Otherwise external people could simply extract credentials by modifying a workflow/Jenkinsfile and open a PR. But since I'm not a committer in xText my changes are not taken into account immediately.

I have two GitHub actions users for that, that's how I experimented for my original solution in a small example ;)
anyway, I tried creating HannesWell#5 let's see how it goes (you need to approve it)

[HannesWell]

I have two GitHub actions users for that, that's how I experimented for my original solution in a small example ;)
anyway, I tried creating HannesWell#5 let's see how it goes (you need to approve it)

Yes I thought about that as well, but for this I was a bit lazy and just asked you. :)
Thanks, just approved it and the result looks good. :)

Concerning inlining the scripts, I think I was told to leave them there. I don't have strong opinions on that. We don't even use them in GitHub Actions anyway. I leave the decision to @cdietrich .

Understand, then @cdietrich can you please decide?

The inlining of the scripts is the only open point in this PR. @cdietrich would that be ok as described above?
But I can also do that in a follow up.

[cdietrich]

what inlining are you referring to?

[HannesWell]

what inlining are you referring to?

My suggestion from above

Furthermore I would like to suggest to inline the scripts used in the Jenkins piplines. That would make the pipelines simpler to understand since there is less indirection. If they are also intended to be run by developers I suggest to use a corresponding M2E Maven launch configuration instead. This has the advantage that it works on all platforms and not only on Linux (and mac?) or if one has cygwin installed.

In general there are many scripts in the repo root and multiple seem to serve the same purpose and I wonder if they are still used. If they are only used in the Jenkinsfile I suggest to inline them and reconsider if they are used locally. For example I would assume that signing is only performed in Jenkins and therefore is no need to specify a separate script for that.

[cdietrich]

@HannesWell i did not get what exactly you want to change.
this is why i am asking.
i am also not sure what you mean by inline e.g. for eclipseVersion

[HannesWell]

@HannesWell i did not get what exactly you want to change. this is why i am asking.

OK, sorry I didn't understand that.

I'm suggesting instead of calling for example full-build.sh in the Jenkinsfile:
sh "./full-build.sh --tp=${selectedTargetPlatform()}"
To just call maven directly

          sh """
            mvn -B clean deploy \
              -f org.eclipse.xtext.full.releng \
              -PuseJenkinsSnapshots \
              -Dtarget-platform-classifier=xtext-${selectedTargetPlatform()}
              -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn \
              -Dit-archetype-tests-skip=true \
              -DaltDeploymentRepository=local::default::file:./build/maven-repository
          """

Delete the script and for local builds to use a M2E Maven launch config.
The same can be done for the deploy script and the other maven-build.sh, tycho-sign/test.sh.

[cdietrich]

so you want to basically change the Jenkins file to call mvn -something
this is fine for me. of course if the bash script does magic calculations (did not check) we also need to do this in jenkinsfile then.

@LorenzoBettini do we have jenkinsfiles for the nightly and the milestones already?

[LorenzoBettini]

@cdietrich @HannesWell currently we have 3 Jenkinsfile that are active

• the one in the root folder (for standard CI)
• /xtext-parent/jenkins/nightly-deploy/Jenkinsfile for nightly
• /xtext-parent/jenkins/milestone-deploy/Jenkinsfile

I don't know if inlining the scripts is a good idea now... at least, I wouldn't touch full-deploy.sh.
full-build.sh is used only in the main Jenkinsfile so that might be inlined.

In the meantime, I'd merge this PR, which seems to work like a charm :)

[HannesWell]

@cdietrich @HannesWell currently we have 3 Jenkinsfile that are active

* the one in the root folder (for standard CI)

* /xtext-parent/jenkins/nightly-deploy/Jenkinsfile for nightly

* /xtext-parent/jenkins/milestone-deploy/Jenkinsfile

I don't know if inlining the scripts is a good idea now... at least, I wouldn't touch full-deploy.sh. full-build.sh is used only in the main Jenkinsfile so that might be inlined.

Thanks for that list, I think the two deploy pipelines can also be merged into one, but I'll be happy to discuss that in a dedicated follow-up PR, where I would also do the inlining of the full-buld scripts.

In the meantime, I'd merge this PR, which seems to work like a charm :)

Yes please do so. For me this is ready. :)

I just rebased on top of the current master and found two more occurrences of the JENKINS_URL variable and removed them.

[LorenzoBettini]

@HannesWell Thanks!
Let's wait for the build to finish (just in case).

Thanks for that list, I think the two deploy pipelines can also be merged into one, but I'll be happy to discuss that in a dedicated follow-up PR, where I would also do the inlining of the full-buld scripts.

To be honest, I'd like to keep those two files separate since they do some slightly but important different things. I wouldn't want to maintain a complex Jenkinsfile with lots of conditions and the likes ;)

[szarnekow]

I'm on the same page as Lorenzo. Rather than inlining everything into the Jenkinsfile, I'd rather have an easy way to run the exact same build locally as Jenkins / Gitlab Actions would. So if we have a long chain of mvn profiles/flags/goals that are used in the Jenkinsfile, the shell-script that I could also use locally has some charm. What do others think?

[HannesWell]

Can we please first finish/submit this PR?
I have already prepared a change on top of this, to merge the workflows and, although you are not enthusiastic about the suggestion, would like to show it to you and have the discussion then in that other PR so that we don't mix too much. I believe that would be a net improvement, but you can then still reject the suggestion. :)

[LorenzoBettini]

@HannesWell merged! Thanks!