-
Notifications
You must be signed in to change notification settings - Fork 45
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
Added install action in the Makefile. #29
Conversation
Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
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.
Thanks for submitting this @tbrachkov - this should make installation even easier 👍
Few minor comments:
- If I'm not mistaken, there isn't a need to use
sudo
as/usr/local/bin
should have user read/write permissions. - Do you mind updating
Installation.md
to include a note about usingmake install
?
Thanks!
Makefile
Outdated
BUILD_PATH = .build/release/$(TOOL_NAME) | ||
LIB_INSTALL_PATH = $(PREFIX)/lib/xcdiff | ||
|
||
SWIFT_LIB_FILES = .build/release/AEXML.swiftdoc .build/release/AEXML.swiftmodule .build/release/Basic.swiftdoc .build/release/Basic.swiftmodule .build/release/PathKit.swiftdoc .build/release/PathKit.swiftmodule .build/release/SPMLibc.swiftdoc .build/release/SPMLibc.swiftmodule .build/release/SPMUtility.swiftdoc .build/release/SPMUtility.swiftmodule .build/release/XCDiff.swiftdoc .build/release/XCDiff.swiftmodule .build/release/XCDiffCommand.swiftdoc .build/release/XCDiffCommand.swiftmodule .build/release/XCDiffCore.swiftdoc .build/release/XCDiffCore.swiftmodule .build/release/XcodeProj.swiftdoc .build/release/XcodeProj.swiftmodule |
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.
I believe copying the binary to /usr/local/bin
should suffice to get xcdiff
running, the libraries are only needed to build it.
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.
You are right I removed the copy for the SWIFT_LIB_FILES
since it's not needed. 🙌
• Copy only the executable to usr/local/bin. Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
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.
Just tried make install
- it's great! Thanks a lot @tbrachkov.
Makefile
Outdated
mkdir -p $(PREFIX)/bin | ||
cp -f $(BUILD_PATH) $(INSTALL_PATH) | ||
|
||
build: clean |
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.
I wonder, wouldn't be better to leave the opportunity for the incremental build?
install: clean build
mkdir -p $(PREFIX)/bin
cp -f $(BUILD_PATH) $(INSTALL_PATH)
build:
xcrun swift build --disable-sandbox -c release
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.
I like the suggestion. Makes sense👌
Documentation/Installation.md
Outdated
cd xcdiff | ||
``` | ||
|
||
2. Build and copy to `/usr/local/bin ` |
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.
maybe we can trim /usr/local/bin
-> /usr/local/bin
:)
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.
Absolutely sorry for my typos.
…build action, added to the install. Signed-off-by: Todor Brachkov <tbrachkov@gmail.com>
Issue number of the reported bug or feature request: #28
Describe your changes
Added install action in the make file that will first build and then create folders and copy all of the products to:
/usr/local/lib/xcdiff
.Testing performed
Installed via:
make install
.Additional context
This is just a proposal, of what we described in issue #28.
Signed-off-by: Todor Brachkov tbrachkov@gmail.com