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 Buildrequires for fontawesome #4569

Merged
merged 1 commit into from
Sep 21, 2023
Merged

Conversation

ckelleyRH
Copy link
Contributor

This resolves a disparity in installed files between Maven and CMake now that fontawesome is not bundled in the CMake build anymore.

This happens because font-awesome is not available at build time so there is no file to link to for the comparison.

@ckelleyRH
Copy link
Contributor Author

The right file should now be installed to link to, but I still need to create the link for the CMake build somehow.

@ckelleyRH
Copy link
Contributor Author

ckelleyRH commented Sep 18, 2023

Hi @edewata, I think I am partway fixed. I'm not sure what the best way to create the symlink is in cmake as it is conditional on a particular version of Fedora and we don't have access to the nice spec file macros. Any ideas?

The alternative, as this all works at run-time and is failing in a test only, we could simply exclude the fonts using sed like we do for cmake/Maven specific things?

@edewata
Copy link
Contributor

edewata commented Sep 18, 2023

I think we can use find_file() to find the actual file in all possible locations, then create the link in CMake. I don't think we should exclude the font from the test since this might affect the installation for non-RPM-based platforms, so the test is actually catching a real build issue. Sorry I didn't check Azure before merging that commit.

@ckelleyRH
Copy link
Contributor Author

@edewata - I'm a bit confused by this now, as ln is claiming that the file exists already, but I don't see how that can be possible as the whole point of this change is to fix that it doesn't exist. Can you take a look please?

@edewata
Copy link
Contributor

edewata commented Sep 21, 2023

Looks like the link to fontawesome-webfont.woff was created and installed successfully by CMake:
https://github.com/dogtagpki/pki/actions/runs/6260272159/job/16998012354#step:7:14612
but the RPM spec is still trying to create the link again in the same location:
https://github.com/dogtagpki/pki/actions/runs/6260272159/job/16998012354#step:7:14872
I think you just need to remove these lines:

pki/pki.spec

Lines 960 to 969 in c8fd8aa

%if %{with theme}
# create links to FontAwesome fonts
%if 0%{?fedora} > 38
ln -s ../../../fonts/fontawesome4/fontawesome-webfont.woff \
%{buildroot}%{_datadir}/pki/common-ui/fonts/fontawesome-webfont.woff
%else
ln -s ../../../fonts/fontawesome/fontawesome-webfont.woff \
%{buildroot}%{_datadir}/pki/common-ui/fonts/fontawesome-webfont.woff
%endif
%endif

so now the link will be created by CMake (which will work on all platforms) instead of by rpmbuild (which will only work on certain platforms).

@ckelleyRH
Copy link
Contributor Author

so now the link will be created by CMake (which will work on all platforms) instead of by rpmbuild (which will only work on certain platforms).

Ah, yes I've misunderstood what was happening, thanks for the clarification! I have fixed the merge conflict and I think now the CI will pass.

@ckelleyRH
Copy link
Contributor Author

@edewata - yep, it works, thanks again!

Copy link
Contributor

@edewata edewata left a comment

Choose a reason for hiding this comment

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

Thanks for the update! There's a test failure, but feel free to fix and merge.

@@ -15,6 +15,8 @@ add_custom_command(
COMMAND ln -sf ../../../../../..${DATA_INSTALL_DIR}/common-ui/ocsp links/
COMMAND ln -sf ../../../../../..${DATA_INSTALL_DIR}/common-ui/pki.properties links/pki.properties
COMMAND ln -sf ../../../../../..${DATA_INSTALL_DIR}/common-ui/tks links/
COMMAND ${CMAKE_COMMAND} -E make_directory fonts
COMMAND ln -sf ../../../../../..${DATA_INSTALL_DIR}/common-ui/fonts/${FONTAWESOME_WEBFONT} fonts/fontawesome-webfont.woff
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like rpminspect failed because the link is incorrect. The final location for the link is /usr/share/pki/common-ui/fonts so it only needs 5 ..'s.

This resolves a disparity in installed files between rpmbuild and cmake
now that fontawesome is not bundled anymore. The symlink generation to
the external font files is moved to cmake from the spec file to make it
platform agnostic.
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
2.1% 2.1% Duplication

@ckelleyRH
Copy link
Contributor Author

Thanks @edewata !

@ckelleyRH ckelleyRH merged commit d3ab85b into dogtagpki:master Sep 21, 2023
150 of 151 checks passed
@ckelleyRH ckelleyRH deleted the font branch September 21, 2023 20:38
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.

2 participants