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

[C#]Change to ENABLE_SPAN_T that doesn't require UNSAFE_BYTEBUFFER. #6073

Merged
merged 5 commits into from
Sep 23, 2020
Merged

[C#]Change to ENABLE_SPAN_T that doesn't require UNSAFE_BYTEBUFFER. #6073

merged 5 commits into from
Sep 23, 2020

Conversation

harujoh
Copy link
Contributor

@harujoh harujoh commented Aug 16, 2020

A small change removes the need for UNSAFE_BYTEBUFFER when using ENABLE_SPAN_T.

@google-cla
Copy link

google-cla bot commented Aug 16, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@harujoh
Copy link
Contributor Author

harujoh commented Aug 16, 2020

@googlebot I signed it!

@harujoh
Copy link
Contributor Author

harujoh commented Aug 16, 2020

Since Span does not necessarily require unsafe, I don't think this should be forced.

When I read the source code, I noticed that the dependency on unsafe was so small that I could break this dependency.

I will make a pull request for that change, so please check it.

@aardappel aardappel requested a review from dbaileychess August 17, 2020 18:42
@google-cla
Copy link

google-cla bot commented Sep 15, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

3 similar comments
@google-cla
Copy link

google-cla bot commented Sep 15, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Sep 15, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Sep 15, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@harujoh
Copy link
Contributor Author

harujoh commented Sep 15, 2020

There are conflicts and rebasing didn't seem to work and I'm getting errors,
but I don't know what to do from here.

Please help me.

@ConanChenTookMyName
Copy link
Contributor

ConanChenTookMyName commented Sep 16, 2020

@harujoh It looks like you merged changes from the official/upstream repository on top of your "ENABLE_SPAN_T doesn't require UNSAFE_BYTEBUFFER." (ac9d852) commit. And then pushed more commits after that. That's why you're getting more comments from google-cla bot.

I think the first thing you should do is make a copy of your local repository folder to create a backup. You'll need to use some destructive git commands to fix your history. If you're using the GitHub Desktop client, I don't think it has the commands you'll need in its UI, so you'll need to use the command line, or another git client.

What I would do is...

  1. Create a temporary branch at "ENABLE_SPAN_T doesn't require UNSAFE_BYTEBUFFER." (ac9d852).
  2. Switch to the temporary branch.
  3. Rebase the temporary branch onto master branch. (The temporary branch will now be up-to-date with master branch.)
  4. Cherry-pick each of your later commits, starting with "Selectable framework." (16088c4), and ending with "Added run on .Net Core." (052b39c). (The temporary branch will now have the correct history with all your commits.)
  5. Switch to ManagedEnableSpanT branch.
  6. Hard-reset ManagedEnableSpanT branch to the latest commit from the temporary branch. (The ManagedEnableSpanT branch will now have the correct history with all your commits.)
  7. Force-push the ManagedEnableSpanT branch to your GitHub repository. (Your pull request should now be fixed.)

If you make any mistakes, you can delete your local repository, and copy the backup to replace it.

@google-cla
Copy link

google-cla bot commented Sep 16, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@harujoh
Copy link
Contributor Author

harujoh commented Sep 16, 2020

@ConanChenTookMyName Thanks for the advice.

You've helped me to create new branches from commits and learn new commands such as Cherry-pick, Hard-reset, Force-push.

However, I think I've followed your advice, but I'm still not getting the rebase working?

The first failure in this problem was despite the fact that I selected [Rebase and merge] from the web. Rebase did not work at all.

@google-cla
Copy link

google-cla bot commented Sep 16, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

2 similar comments
@google-cla
Copy link

google-cla bot commented Sep 16, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Sep 16, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ConanChenTookMyName
Copy link
Contributor

@harujoh I'm happy to help.

It looks like your master branch's history is wrong. I think I overlooked this before. You have a series of commits authored by other people, but have you as the committer. That's why google-cla bot keeps asking for consent—because it needs consent from the original authors (because the commit ID's changed from the original). This issue was probably caused by the the original rebase/merge from the web UI you mentioned.

You need to get your master branch back in-sync with the official/upstream repository first. Do you have a second remote added to your local repository, pointing to the official/upstream repository? If not, you need to add it first. (So, you should have an "origin" remote pointing to your fork on GitHub, and an "upstream" remote pointing to Google's repository.)

Then...

  1. Switch to your local master branch.
  2. Hard-reset back to "Adds a serialize helper function to native table" (63cc0ee).
  3. Pull upstream's master branch into your local master branch (make sure this performs a merge, not a rebase). (Your local master branch will now be in-sync with upstream's master branch.)
  4. Force-push your local master branch to your fork on GitHub.
  5. Switch to your ManagedEnableSpanT branch.
  6. Rebase ManagedEnableSpanT branch onto your master branch.
  7. Force-push your ManagedEnableSpanT branch to your fork on GitHub.

I tried this using a local checkout of your repository, and it looks like it should fix the issues with its Git history.

Change to ENABLE_SPAN_T that doesn't require UNSAFE_BYTEBUFFER.
Changed target framework to allow selection of 2.0 or 2.1 (or higher)
@harujoh
Copy link
Contributor Author

harujoh commented Sep 17, 2020

@ConanChenTookMyName Thank you so much!

You have helped me to reach my goal.

It must have been very difficult to read the English that relied on machine translation.
Nevertheless, thank you so much for sticking with me until the problem was solved.

Upstream didn't know it existed. Thanks to you I was able to learn about it.

But unfortunately, I think I had some errors in the flag and I'm going to try to fix them now.

Finally, again, thank you so, so much for your help.
I am truly grateful for this encounter.

@ConanChenTookMyName
Copy link
Contributor

@harujoh The machine translation was very clear. I'm glad you were able to understand my writing too. I'm happy to see we were able to fix the problem. It will be your turn to help someone else with Git one day.

@harujoh
Copy link
Contributor Author

harujoh commented Sep 19, 2020

I was not good at git flow.
I am so grateful for the assistance I got from you to challenge it.

I am firmly committed to passing the relay baton you gave me to someone else.

There is not encouraged to waste time here, so I'll end my reply with this sentence.
I've repeated myself many times, but let me just say one last thing.

Thank you so much.
ありがとうございました

@dbaileychess
Copy link
Collaborator

Sorry for the confusion with the conflicting merges, we will get it all sorted out. Thanks for the contributions!

@dbaileychess dbaileychess merged commit e0bbaa6 into google:master Sep 23, 2020
@harujoh harujoh deleted the ManagedEnableSpanT branch September 24, 2020 00:37
@Shixiaowei02
Copy link
Contributor

Shixiaowei02 commented Sep 24, 2020

Due to the problem of the newline format, after pulling the latest master branch, it is not possible to switch to the old commit on the unix platform. Can this problem be solved through the .gitattribute or other configuration of the repository code? Now it has caused a lot of trouble for our continuous generation test. Thank you!

% git checkout v1.12.0
error: Your local changes to the following files would be overwritten by checkout:
	tests/FlatBuffers.Test/FlatBuffers.Core.Test.csproj
Please, commit your changes or stash them before you can switch branches.
Aborting

@dbaileychess @harujoh

@anassinator
Copy link
Contributor

@harujoh @dbaileychess This breaks usage of ENABLE_SPAN_T + UNSAFE_BYTEBUFFER on NET Standard 2.0. It spews a bunch of errors like:

flatbuffers\net\Flatbuffers\ByteBuffer.cs(586,13): error CS0103: The name 'WriteLittleEndian' does not exist in the current context
flatbuffers\net\Flatbuffers\Table.cs(109,51): error CS1061: 'ByteBuffer' does not contain a definition for 'ToSpan' and no accessible extension method 'ToSpan' accepting a first argument of type 'ByteBuffer' could be found (are you missing a using directive or an assembly reference?)
flatbuffers\net\Flatbuffers\FlatBufferBuilder.cs(219,38): error CS1503: Argument 2: cannot convert from 'System.Span<T>' to 'byte'

I think the #ifdefs might not be doing the equivalent thing when NETSTANDARD2_1 is not set

@harujoh
Copy link
Contributor Author

harujoh commented Sep 25, 2020

@anassinator Thanks for noticing the glitch.

I've created a pull request that fixes the problem, so please wait for it to be merged

#6137

@dbaileychess
Copy link
Collaborator

Merged #6137, @anassinator sorry for the churn.

@harujoh
Copy link
Contributor Author

harujoh commented Sep 25, 2020

@Shixiaowei02 Thanks for noticing the glitch and fixing it.

Sorry for the late reply.
Since I was checking notis from cell phone, I only read what was posted later and missed your post.

Normally the line breaks should have been automatically converted to follow the project, but for some reason they weren't.
I'm a regular user of Windows, so I wasn't aware of this problem.

@dbaileychess This pull request is very good, with my line break code error corrected.
Please merge them together as soon as possible.

#6133

@Shixiaowei02
Copy link
Contributor

@Shixiaowei02 Thanks for noticing the glitch and fixing it.

Sorry for the late reply.
Since I was checking notis from cell phone, I only read what was posted later and missed your post.

Normally the line breaks should have been automatically converted to follow the project, but for some reason they weren't.
I'm a regular user of Windows, so I wasn't aware of this problem.

@dbaileychess This pull request is very good, with my line break code error corrected.
Please merge them together as soon as possible.

#6133

It is my pleasure. Thanks. @dbaileychess @harujoh

ivannp pushed a commit to ivannp/flatbuffers that referenced this pull request Oct 2, 2020
…oogle#6073)

* ENABLE_SPAN_T doesn't require UNSAFE_BYTEBUFFER.

Change to ENABLE_SPAN_T that doesn't require UNSAFE_BYTEBUFFER.

* Selectable framework.

Changed target framework to allow selection of 2.0 or 2.1 (or higher)

* Added target framework version check.

* Add core test project.

* Added run on .Net Core.
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.

5 participants