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

Revert "Add an option to add CPUID tag to sysimg and .ji files" #19821

Closed
wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Sponsor Member

@vtjnash vtjnash commented Jan 2, 2017

Reverts #19486

Precompile will get confused by this. One of the assumptions it needs is that all clients must use the same sysimg as on node one to prevent the nodes from throwing "out-of-sync" errors when trying to load packages.

@Keno
Copy link
Member

Keno commented Jan 2, 2017

That's only a problem if you're adding heterogeneous nodes to the same julia-level cluster. Note that this also namespaces the .ji files.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

Currently all code is loaded from node 1 and should be ignoring the local file system. I know we are looking at variations of that, but they aren't merged (or ready) yet.

We already have a functioning heterogeneous cluster option (generic binaries).

@JeffBezanson
Copy link
Sponsor Member

JeffBezanson commented Jan 2, 2017

I don't think nodes should load packages from node 1. Nodes should only need to be wire-protocol-compatible, not necessarily need to all have the same architecture/os. Generic binaries aren't sufficient, since one of the problems you hit is packages with binary dependencies installed in different locations, which can happen with identical architectures but slightly different linux distros.

@Keno
Copy link
Member

Keno commented Jan 2, 2017

So don't use this option with heterogeneous julia-level clusters. I have yet to be given an actual reason why we shouldn't have this option for what it's good for.

@JeffBezanson
Copy link
Sponsor Member

Since the option is off by default I don't see why it would be urgent to revert.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

@JeffBezanson agreed, but we should demonstrate and merge that other PR first

So don't use this option with heterogeneous julia-level clusters. I have yet to be given an actual reason why we shouldn't have this option for what it's good for.

The documentation for the option says, this is good for heterogeneous cluster. If it isn't, then why are we adding it?

@Keno
Copy link
Member

Keno commented Jan 2, 2017

The documentation for the option says, this is good for heterogeneous cluster. If it isn't, then why are we adding it?

You can have a heterogeneous cluster without using heterogeneous nodes within the same julia cluster.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

It's mostly off, but it's not entirely hidden behind feature-flags

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

Also, it breaks the string-replace script we use to make binary releases.

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

And it seems it may have broken non-x86 support #19822

@Keno
Copy link
Member

Keno commented Jan 2, 2017

I'm working on a follow up commit to fix #19822, but re

Also, it breaks the string-replace script we use to make binary releases.

Jameson, if you know something is broken, you should tell people exactly what and in what way. Saying "the string-replace script is broken" is not actionable. If you don't tell me what you know I can't fix it.

@tkelman
Copy link
Contributor

tkelman commented Jan 2, 2017

julia/Makefile

Lines 412 to 413 in 1c605d2

$(call stringreplace,$(DESTDIR)$(libdir)/libjulia.$(SHLIB_EXT),sys.$(SHLIB_EXT)$$,$(private_libdir_rel)/sys.$(SHLIB_EXT))
$(call stringreplace,$(DESTDIR)$(libdir)/libjulia-debug.$(SHLIB_EXT),sys-debug.$(SHLIB_EXT)$$,$(private_libdir_rel)/sys-debug.$(SHLIB_EXT))

@Keno
Copy link
Member

Keno commented Jan 2, 2017

Is this broken even if you don't use this option?

@tkelman
Copy link
Contributor

tkelman commented Jan 2, 2017

Believe so, since it's looking for a string that includes the shlib extension but you've separated that. And the max length is no longer consistent.

@Keno
Copy link
Member

Keno commented Jan 2, 2017

The names of the shared libraries should have have changed if this option is not enabled. Did I make a mistake?

@vtjnash
Copy link
Sponsor Member Author

vtjnash commented Jan 2, 2017

It's also missing the null padding at the front. If you want more actionable comments, bump the PR, rather than merging it. I can only type so much on a phone.

@tkelman
Copy link
Contributor

tkelman commented Jan 2, 2017

stringreplace looks for an actual string (the value of JL_SYSTEM_IMAGE_PATH) inside the binary using strings

@Keno
Copy link
Member

Keno commented Jan 2, 2017

The PR has been sitting there with an approved review for the last two weeks. That was the time for review. Given that didn't happen I merged it. I will happily consider and address any and all comments you may have.

@tkelman ah, I see what you're saying

@Keno
Copy link
Member

Keno commented Jan 2, 2017

I have addressed the stringreplace issue in #19823.

@tkelman
Copy link
Contributor

tkelman commented Jan 5, 2017

@vtjnash if there's anything else that #19823 didn't fix, please open an issue or (replace this) PR

@tkelman tkelman closed this Jan 5, 2017
@tkelman tkelman deleted the revert-19486-kf/cpuidtag branch January 5, 2017 22:19
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.

4 participants