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

Fix #1157: Implement versioned library naming #1268

Closed
wants to merge 8 commits into from

Conversation

lht
Copy link
Contributor

@lht lht commented Dec 7, 2011

Green on all bots.
#1157

@brson
Copy link
Contributor

brson commented Dec 7, 2011

This looks good to me so far. A few comments:

  • This seems to generate both the libfoo.so and libfoo-hash-vers.so. Instead, can we just generate the hashed filename and call 'touch libfoo.so' in the build rules to satisfy make?
  • We need to update 'make clean' to delete these hash-based libraries
  • We need to update the snapshot scripts to handle hash-based libraries
  • I'm worried about what happens when we've globbed-copied these libraries around for a while. It seems like we'll eventually end up with multiple libraries with the same version but different hashes.

@lht
Copy link
Contributor Author

lht commented Dec 8, 2011

Thanks for reviewing! I'll update and resent later!

@lht
Copy link
Contributor Author

lht commented Dec 8, 2011

Updated with all comments addressed. (except the last one)

(Looks like github dislike rebasing of pull requests. All inline comments given to the previous pull request are no longer listed on this page. Or, do we prefer close old pull request and open new one in such case?)

I've also removed the change about including hashes of dependencies from my previous pull request. Because there is not discrimination about direct and indirect dependencies in the current implementation. I would prefer only include hashes of direct dependent crates, and encoding hash and hashes of direct dependencies into crates metadata. There are a few other details I'm not clear of. So I think it's better to start a new series on that.

One thing in particular is what should be hashed? According to comments about build_link_meta:

 *  - Define CMETA as all the non-name, non-vers exported meta tags in the
 *    crate (in sorted order).
 *
 *  - Define CMH as hash(CMETA).
 *

If version is not hashed, I wonder how can it be transited across the dependency DAG.

@graydon
Copy link
Contributor

graydon commented Dec 8, 2011

Version is not hashed in order to permit backward compatible libraries to be upgraded without causing all dependant libraries to be recompiled. If the user wishes to force that they can change any extra link metadata tags.

This is a compromise between the desire to control and accurately reflect dependency, and the desire to do upgrades in the field the way os vendors prefer, using major.minor style versioning.

@lht
Copy link
Contributor Author

lht commented Dec 11, 2011

Another issue, for crates with different names and without extra meta data, they may end up the same hash. I think it would be good to use hash as a unique id when resolving crate dependencies. Actually, there are more than one FIXMEs about replacing the cnum with hash.

Is there any background about not including crate name in hash?

lht added 8 commits December 12, 2011 17:46
Also updated build to install versioned libraries and added a few
missing actions for `make clean`.
On Linux/Mac, I got a segmentation fault:

  (gdb) bt
  #0  0x00000000007519af in glue_take584 ()
  rust-lang#1  0x00000000006d4bec in
    back::rpath::get_rpath_flags::_3899df2ca513c603 ()
  rust-lang#2  0x00000000006c7655 in back::link::link_binary::_7afde00a9791031c ()
  rust-lang#3  0x00000000007d3ff5 in driver::rustc::compile_input::thunk9212 ()
  rust-lang#4  0x0000000000710f24 in driver::rustc::time::_3e691b2a4ba58aee ()
  rust-lang#5  0x000000000071a79d in
    driver::rustc::compile_input::_7b4a41b87c18e034 ()
  rust-lang#6  0x000000000072f0a9 in driver::rustc::main::_cd8b8c8185af3dee ()
  rust-lang#7  0x000000000072f1ed in _rust_main ()
  rust-lang#8  0x00007ffff7e6e146 in task_start_wrapper (a=<optimized out>) at
    ../src/rt/rust_task.cpp:176

The variable `output` or `out_filename` becomes (null) after the definition
of `fn unlib`. Move the function defintion to the beginning seems
prevent the crash on Linux.
@brson
Copy link
Contributor

brson commented Dec 12, 2011

This looks really good.

@graydon
Copy link
Contributor

graydon commented Dec 13, 2011

Pushed to master, working through first snapshot now. Here's hoping!

@graydon
Copy link
Contributor

graydon commented Dec 13, 2011

Appears to have stuck with a couple little hacks to make the snapshots load right. Further work on getting version numbers to make sense given OS conventions will be done on branch. Thanks, this is great!

@graydon graydon closed this Dec 13, 2011
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Aug 24, 2022
Cranelift 0.87 now follows its own documentation regarding
shift amounts, and implicitly masks them if the arch requires it. [0]

[0]: bytecodealliance/wasmtime@0508932
bjorn3 added a commit to bjorn3/rust that referenced this pull request Aug 24, 2022
celinval pushed a commit to celinval/rust-dev that referenced this pull request Jun 4, 2024
* docs: fix typos

* docs: revert UB repetition because it's used in different contexts

Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
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.

3 participants