Skip to content

Conversation

@omochi
Copy link

@omochi omochi commented Nov 15, 2024

This patch enables building OpenXLSX with SwiftPM.
SwiftPM is the package manager for the Swift programming language.
It can define packages implemented not only in Swift but also in C or C++, making it possible to use OpenXLSX from Swift by leveraging Swift’s C++ interoperability features.

Broadly, the following some changes were required:

(1) Adding Package.swift and modulemap

A SwiftPM package requires the Package.swift file to be located directly under the root directory of the Git repository.
Additionally, C++ targets require a modulemap to enable Clang module support.

(2) Fixing the copy constructor of XLCellAssignable

Swift’s C++ interoperability requires that the copy constructor of the type being used is correctly defined.
In the copy constructor of XLCellAssignable, the argument type was XLCell instead of XLCellAssignable, which caused the Swift compiler to throw an error. This patch addresses and fixes this issue.

I have removed this change.

(3) Changing header file includes from angle brackets to double quotes

In my configuration design, I set the header root of the OpenXLSX Clang module to correspond to the OpenXLSX/ directory.
When headers within the module include other headers, using angle brackets caused errors, and it was necessary to use double quotes instead.

(4) Pre-generating export headers and indirect inclusion

SwiftPM uses the repository directly as a package, but it cannot perform pre-processing steps such as running CMake. Therefore, it is necessary to either define settings in Package.swift or eliminate the need for CMake processing.
In particular, the export header OpenXLSX-Exports.hpp cannot be generated dynamically, so it must be pre-generated and committed to the repository.

To avoid altering the behavior of existing CMake builds, I implemented an indirect inclusion mechanism: when building with CMake, the generated header is included, and otherwise, the pre-generated header is used.
Instead of including OpenXLSX-Exports.hpp directly, I created a wrapper header file, include-export-header.hpp, which determines which export header to include based on the build context.


endif()

target_compile_definitions(OpenXLSX PUBLIC OPENXLSX_CMAKE)
Copy link
Author

Choose a reason for hiding this comment

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

To address (4), I added a flag to indicate that the build was performed using CMake.


// ===== OpenXLSX Includes ===== //
#include "OpenXLSX-Exports.hpp"
#include "include-exports-header.hpp"
Copy link
Author

Choose a reason for hiding this comment

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

As part of addressing (4), I modified the code to include the indirect header.

* @param other the cell to construct from
*/
XLCellAssignable (XLCell const & other);
XLCellAssignable (XLCellAssignable const & other);
Copy link
Author

Choose a reason for hiding this comment

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

This is part of the changes made to address (2).

// ===== pugixml.hpp needed for pugi::impl::xml_memory_page_type_mask, pugi::xml_node_type, pugi::char_t, pugi::node_element, pugi::xml_node, pugi::xml_attribute, pugi::xml_document
#include <external/pugixml/pugixml.hpp> // not sure why the full include path is needed within the header file
#include <XLException.hpp>
#include "XLException.hpp"
Copy link
Author

Choose a reason for hiding this comment

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

This is part of the changes made to address (3).

@@ -0,0 +1,6 @@
module CxxOpenXLSX {
Copy link
Author

Choose a reason for hiding this comment

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

The Clang module name is prefixed as CxxOpenXLSX. This is because, in Swift, situations can arise where having the same name for a module and a type contained within that module becomes inconvenient.
Since the library includes the OpenXLSX namespace, which is imported as a top-level type in Swift, the module name is adjusted to avoid conflicts with it.
Additionally, it is a common convention in Swift to use the Cxx prefix for C++ targets.

],
targets: [
.target(
name: "CxxOpenXLSX",
Copy link
Author

Choose a reason for hiding this comment

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

The target name must be the same as the Clang module name.

@omochi
Copy link
Author

omochi commented Nov 15, 2024

This library is a Swift wrapper for OpenXLSX that I am currently developing. It makes use of this patch.

@aral-matrix
Copy link
Collaborator

While for the general decision on such a support I would delegate to @troldal, since it is his project, looking at your proposed changes there is too much and there are misunderstandings on how some parts of the code work, so I personally won't be able to work on anything you propose - specifically the intended use case for the copy constructor of XLCellAssignable is to copy-construct from a regular XLCell, you can't just change that without breaking existing functionality. Unfortunately, your proposal is not a fix, but introduces a bug.

Also, changing the includes because your environment is (mis)configured to produce errors is not an approach I would follow. There is no guaranteed functional difference between quotes and angle brackets for include-statements. Some compilers use one (typically quotes) to search the local folder before any components of the include path, others do not. So if you get error messages with angle brackets, chances are that you misconfigured the include paths.

@aral-matrix aral-matrix requested a review from troldal November 15, 2024 15:21
@aral-matrix aral-matrix added the help wanted Extra attention is needed label Nov 15, 2024
@omochi
Copy link
Author

omochi commented Nov 16, 2024

Thank you for your prompt comments.

I will take a closer look when I have more time later.

For now, it seems I misunderstood the purpose of XLCellAssignable, so I will investigate whether I can achieve my goal without making changes to it.

@aral-matrix
Copy link
Collaborator

In case this helps your understanding:
I created XLCellAssignable to provide a convenient way to store a value in a cell like so: wks.cell("A5") = 0.123; and to copy cell contents like with Office copy & paste functionality like so: wks.cell("A5") = wks.cell("A6"); (copies formula, value and formatting).

In order to be able to overload the assignment operators, I could not change the XLCell class because XLCell assignment operators have a different purpose which is used in the library: Assigning an XLCell variable to another XLCell variable copies the reference to the specific position in the worksheet.

In the end, the use case for XLCellAssignable is to be returned by the XLWorksheet::cell functions, so that you can directly assign a value, or copy cell contents as per the above examples.

The only foreseen way to construct an XLCellAssignable is to copy-construct or move construct from an XLCell object, and the only time the library constructs it is in XLCellAssignable XLWorksheet::cell(uint32_t rowNumber, uint16_t columnNumber) const

There is no use case for constructing an XLCellAssignable from an XLCellAssignable - and to make that clear, I did not implement the copy/move constructors for the classes own type.

@omochi
Copy link
Author

omochi commented Nov 18, 2024

I reverted the incorrect changes to XLCellAssignable .
I will remove the related content from the initial comment of the pull request.

@omochi
Copy link
Author

omochi commented Nov 18, 2024

I have created a standalone patch(#298) for the changes in (3) and included a detailed explanation. Could you please review it?

@omochi
Copy link
Author

omochi commented Dec 4, 2024

I accidentally added unnecessary commits, so I’ll close this PR for now.

@omochi omochi closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants