-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix/win make #4682
Fix/win make #4682
Conversation
I thought the makefiles tried to detect automatically if gx was already installed and use it if it was, I didn't notice With these commits I'm able to clone the repo, set environment variables, build, and install |
@victorbjelkholm What is the latest on windows CI through jenkins? |
Cygwin's
Now this only happens when For now we can just avoid linking altogether on Windows and just copy the binary every time. It's not ideal but I can't think of a way around it. |
PR in progress is here: #4430 Windows tests seems to work fine, but it's not calling |
bin/Rules.mk
Outdated
@@ -1,6 +1,10 @@ | |||
include mk/header.mk | |||
|
|||
dist_root_$(d)=/ipfs/QmT3CLJKJzWPuN4NAN4LLy69UpKskMF3AuYhXstKdn8V43 | |||
binpostfix := |
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.
We already have binpostfix
in this makefile system. You can use $(?exe)
to access it.
bin/Rules.mk
Outdated
rm -f $@ | ||
ln -s $(notdir $^) $@ | ||
rm -f $@$(binpostfix) | ||
ifeq ($(OS),Windows_NT) |
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.
Same with this, use the variable exported here: https://github.com/ipfs/go-ipfs/blob/master/mk/util.mk#L4
mk/gx.mk
Outdated
ifeq ($(OS),Windows_NT) | ||
binpostfix = .exe | ||
endif | ||
gx-path = gx/ipfs/$(shell gx$(binpostfix) deps find $(1))/$(1) |
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.
gx
in this file should be avialable from PATH variable so path or the postfix should not be needed.
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.
Does it work without 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.
As in the diff comments.
@Kubuxu |
@magik6k could you look why Jenkins failed here? |
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.
Makefiles seem good. I would just love to know why Jenkins failed.
It's a random test fail, I've seen it a few times, reported in #4698 (also re-run jenkins, it's green now) |
@djdv can you try running From another side, I don't see a reason this couldn't get merged. It improves the situation, it just might not be perfect. |
License: MIT Signed-off-by: Dominic Della Valle <[email protected]>
@Kubuxu Are you encountering double unpacking? Also I will force push to restart the test if they're working now. I needed to amend the commit anyway (wrong email address was attached originall). No code changes. |
@djdv no, but this is something I suspected that could happen reading the Makefiles. @whyrusleeping good to merge |
Thank you @djdv! Its really great to have someone holding the windows flag :) |
I can't wait for the day when we can depend on the WSL and cross compile for windows on windows... |
Requesting review from @Kubuxu I'm not very familiar with IPFS's
make
rules ormake
in general so this may need changes.In my testing this ~~~resolved~~~ partially resolves #4510 when using MSYS2 tools.
I'm going to look into gx detection
While working on this I noticed a problem I couldn't work around.
/bin/Rules.mk utilizes a symlink for the binary dependencies, on Windows, by default, users don't have permission to create symlinks with
mklink
, MSYS2'sln.exe
simply copies the source to the target (not as a link). This ends up not being a problem in practice but I feel the need to mention it. Here's the patch I initially tried without success.