-
Notifications
You must be signed in to change notification settings - Fork 30
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
#337 add mac architectures #376
#337 add mac architectures #376
Conversation
added architectures to OSs (default x64) added mac-arm architecture
added automatic detection of os and architecture
Pull Request Test Coverage Report for Build 9840802466Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jan-vcapgemini thanks for this PR. If I understood this approach correctly, there are issues that Java/Maven reports the architecture not properly normalized (for system property os.arch
) so the same thing can get different values depending on the platform.
Therefore insted of profiles triggered by OS activation setting things like this:
<os.detected.classifier>windows</os.detected.classifier>
<os.detected.arch>x64</os.detected.arch>
You now use os-maven-plugin to generate such properties in a normalized way. While this seems like a smart and valid approach, you need to be aware that the previous properties where somehow made up by some developer in our team. Even though os-maven-plugin uses the same property names, it does use them not with the same semantics.
IMHO you need to change at least
<releaseName>${project.artifactId}-${os.detected.classifier}-${os.detected.arch}</releaseName>
to
<releaseName>${project.artifactId}-${os.detected.classifier}</releaseName>
or to
<releaseName>${project.artifactId}-${os.detected.name}-${os.detected.arch}</releaseName>
if I read the documentation of that plugin correctly.
Further, we have standardized our architectures differently and use x64
instead of x86_64
so we still might need to find a way how to remap these but this is some nice to have that can also be addressed after merging this PR.
moved os plugin location (was not being used properly) adjusted release name
# Conflicts: # cli/pom.xml
Can be closed because #471 was already merged. |
Fixes: #337
Implements: