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.

Sign up for GitHub

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

Jump to bottom

fp_dataset: Fix hex representation of most negative value in floatingPoint_tohex() #85

Merged
merged 1 commit into from
Apr 9, 2024
Merged

fp_dataset: Fix hex representation of most negative value in floatingPoint_tohex() #85

merged 1 commit into from
Apr 9, 2024

Conversation

cmuellner
Copy link

A value of "0x0xFFEFFFFFFFFFFFFF" does not make sense.
Fix that to "0xFFEFFFFFFFFFFFFF".

@cmuellner
Copy link
Author

Ping @pawks @allenjbaum @neelgala

allenjbaum
allenjbaum previously approved these changes Apr 1, 2024
View reviewed changes
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smallobvios fix

@allenjbaum
Copy link
Collaborator

this failed the version check, I ?think? - but the version string has been updated, so no idea what is going on.

@cmuellner
fp_dataset: Fix hex representation of most negative value in floating…
f581d8d 
…Point_tohex()

A value of "0x0x..." does not make sense.
Fix that.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
@cmuellner
Copy link
Author

The error message is "Versions are not equal in Changelog and init.py.".
The version in riscv_isac/__init__.py needed to be updated as well.

I've updated the PR accordingly.

allenjbaum
allenjbaum approved these changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple fix (though not obvious that the changlelog has to be updated in both places....

@allenjbaum allenjbaum merged commit 1bdc0a7 into riscv-software-src:master Apr 9, 2024
1 check passed
@allenjbaum
Copy link
Collaborator

allenjbaum commented Apr 9, 2024 via email

@cmuellner
Copy link
Author

It continues to fail, a bit more spectacularly this time: Node.js 16 actions are deprecated. Please update the following actions to use Node.js 20: @., @. For more information see: https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/ . The following actions uses node12 which is deprecated and will be forced to run on node16: @., @. For more info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/ The set-output command is deprecated and will be disabled soon. Please upgrade to using Environment Files. For more information see: https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

I don't see where node.js comes into play here (I don see how to access the complete log).
The code is implemented in Python and the CI workflows build upon ubuntu-latest.
Maybe the issue comes from using image: python:3.7-alpine in https://github.com/riscv-software-src/riscv-isac/blob/master/.gitlab-ci.yml.

For sure, this is independent of this PR.

@allenjbaum
Copy link
Collaborator

It generated those message, but it's merged now. I guess those are just warnings?

@cmuellner
Copy link
Author

It generated those message, but it's merged now. I guess those are just warnings?

Likely, yes, but still, that should be addressed before it stops working.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Apr 10, 2024 via email

@allenjbaum
Copy link
Collaborator

allenjbaum commented Apr 10, 2024 via email

@allenjbaum
Copy link
Collaborator

OK, the failure turns out to be a CI failure, specifically (I think) this line of code:

    - python -m twine upload --username $pypiusername --password $pypipassword dist/*

That result in this error:
ERROR HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/
Username/Password authentication is no longer supported.
Migrate to API Tokens or Trusted Publishers instead.
See https://pypi.org/help/#apitoken
and https://pypi.org/help/#trusted-publishers

I don't understand why we need to publish packages to pypi (or exactly which package: dist?)
or what would break if we just commented this out.

@cmuellner
Copy link
Author

OK, the failure turns out to be a CI failure, specifically (I think) this line of code:

    - python -m twine upload --username $pypiusername --password $pypipassword dist/*

That result in this error: ERROR HTTPError: 403 Forbidden from https://upload.pypi.org/legacy/ Username/Password authentication is no longer supported. Migrate to API Tokens or Trusted Publishers instead. See https://pypi.org/help/#apitoken and https://pypi.org/help/#trusted-publishers

I don't understand why we need to publish packages to pypi (or exactly which package: dist?) or what would break if we just commented this out.

I suggested merging all ACT-relevant repositories into one repo a long time ago (maybe 1.5-2 years?).
Distributing tightly coupled helper tools like isac via pypi/pip as stand-alone tools is pointless.
These helpers can only be used to generate the ACTs, i.e., they are not generic tools for several purposes.
And even worse, many new ACT PRs depend on changes in these helpers.
So, a local installation via pypi would not be sufficient to build these ACTs.

My suggestion:

  1. Move the code of the riscv-config, riscv-ctg, riscof and riscv-isac repositories into the riscv-arch-test repository
  2. Archive the now obsolete repos
  3. Remove all external distributions of these repos/tools/docs (e.g. PyPI, readthedocs.io)
  4. Migrate the issues of the archived repos
  5. Migrate the PRs of the archived repos

This results in a single repo needing to be maintained for the RISC-V ACTs, which significantly reduces maintenance efforts.

@Timmmm
Copy link

Timmmm commented Jun 7, 2024

@cmuellner I definitely agree. There's no need for so many repos for tools that have to be used together. With perhaps the exception of riscv-config which is intended to be used by other projects (e.g. Sail).

Also, was master force-pushed backwards? This commit was merged: 1bdc0a7

But it seems like master points to the previous commit now. Was this intentional? If so, it's generally a really bad idea to be force-pushing master! Instead you should use git revert and make a new commit that undoes the old one. If not, then Github has settings that prevent you from accidentally force pushing master.

@cmuellner
Copy link
Author

So, I take the time and work on a PR to improve this repo.
Allen then reviews this change and merges it.

Then somebody comes along and force-pushes this fix away without even notifying anyone?
If that was an accident, then we should consider making all branches of this repo "protected" in settings, which means that force-pushing is not possible.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jun 7, 2024 via email

@Timmmm
Copy link

Timmmm commented Jun 7, 2024

Git itself doesn't record this (except locally in the "reflog") but it turns out that GitHub does, in the Activity View:

https://github.com/riscv-software-src/riscv-isac/activity

You can see that @jamesbeyond force-pushed to master 9 days ago. James was that intentional?

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jun 7, 2024 via email

@jamesbeyond
Copy link
Collaborator

Git itself doesn't record this (except locally in the "reflog") but it turns out that GitHub does, in the Activity View:

https://github.com/riscv-software-src/riscv-isac/activity

You can see that @jamesbeyond force-pushed to master 9 days ago. James was that intentional?

Hi @Timmmm , I can confirm this is intentional, the reason is that, we are moving to dev branch development mode,
which means this repo will have 2 branches, dev and main (release)

  • dev this the default branch now, and all PRs should go to dev.
  • when making a release, we will rebase main on to dev, to get then in sync.
  • however on this repo, because this PR got merged in main caused main diverged from dev, which is we don't want, but lucky it is only this is commit and it was not part of any release (pushed onto Pypi),
  • so we make the decision to revert this PR.
  • And this changes in the PR already part of dev branch, so you will find this bug fix in the next release.

All these information had been published in Guoup.io ACT SIG channel, so if you are working on a PRs, please check and update your PR to dev branch

@cmuellner
Copy link
Author

Git itself doesn't record this (except locally in the "reflog") but it turns out that GitHub does, in the Activity View:
https://github.com/riscv-software-src/riscv-isac/activity
You can see that @jamesbeyond force-pushed to master 9 days ago. James was that intentional?

  • And this changes in the PR already part of dev branch, so you will find this bug fix in the next release.

Can you please show me where the commit of this PR landed in the dev branch?
I could not find it here: https://github.com/riscv-software-src/riscv-isac/commits/dev/

Also in the activity log, that @Timmmm mentioned, I can see the merge from @allenjbaum and your force-push. However, there is no activity related to getting the commit of this PR to the dev branch.

All these information had been published in Guoup.io ACT SIG channel

I assume you are referring to the email with the subject "Important Updates to ACT Repositories". This email was sent on May 21, 2024. This PR was created on March 7, 2024, more than 10 weeks before your email was sent. This PR was merged on April 9, 2024 which was more than 6 weeks before that email.

Further, in this email I cannot find any reference that the commits of this PR will be force-pushed away.

so if you are working on a PRs, please check and update your PR to dev branch

Asking me to change a PR 6 weeks after it got merged does not make sense.

Discussions and accusations are a waste of time at this stage. Please simply add the missing commit to the dev branch. Please also double-check if there are other commits in other PRs that fell through the cracks when changing the development process.

@Timmmm
Copy link

Timmmm commented Jun 8, 2024

Yeah I have to agree with @cmuellner. This definitely shouldn't have been done. In future use git revert if you want to revert a commit on a "public" branch like master, and probably dev. It's fine to force-push in a branch that people aren't likely to be using.

Anyway best not to dwell on it. I would recommend protecting the branch in the project settings so it doesn't happen again.

@jamesbeyond
Copy link
Collaborator

Hi @cmuellner @Timmmm,

I have to apology, if the communication to the PR owner was missing before the reset and force push, I sent out a email to Allen (ISA Infra HC Vice-Chair) and Umar (ACT Vice-Chair), maybe I should have cc both of you. I'll forward that email to you. in the emails there was some explanation on the planned action.

I am totally aware of reset and force push a public branch it is a big deal, so we did some checking before making the decision, The main reason for doing a reset and force push, is the main branch and dev branch are diverged , a revert commit is not helping here it will causes further diverging. a rebase also equivalent to a force push.

Regarding to this PR, the only effect change the following line, and updates to the change logs:
image
And this change already made to dev by others' PRs, looking at dev branch latest code :
https://github.com/riscv-software-src/riscv-isac/blob/dev/riscv_isac/fp_dataset.py#L285-L287
Nothing need to be done to the dev branch. So nothing will be required form PR owner to do.


Asking me to change a PR 6 weeks after it got merged does not make sense.

We were trying to make the trasisson transparent, and less impacts where possible, in the Group.IO message, it was a general advise. what I mean there was if people have a open PR not merged yet, please have have a look and update your PR, in order to get them merged, But for the already merged RPs, noting will be required, and we will sort that out.

To summarize on this, thanks for pointing this out, and I feel deeply sorry if this impacts you or your work.
Reset and a force push, it is not ideal but a necessary action we need to do, in order to keep the main and dev in sync, and fixed diverging problem. we already keep the branches protected, and intend to have more CI to guard them.
And I believe, nothing will required from you to do, and please let me know if there is something still not working for you.

@cmuellner
Copy link
Author

Oh, I see. So, someone else already fixed that on the dev branch before.
I was not aware of that. Sorry for my complaint!

And thanks for improving the development process of the RISC-V arch tests!

@Timmmm
Copy link

Timmmm commented Jun 10, 2024

All good.

The main reason for doing a reset and force push, is the main branch and dev branch are diverged , a revert commit is not helping here it will causes further diverging.

Just FYI, it doesn't matter if main and dev diverge. You would normally fix a mistake like this by git reverting it on main and then merge dev into main later, at which point your commit graph would look like this:

* Merge dev into main
|\
| \
|  * Some dev commit
|  |
|  * Some other dev commit
|  |
* Revert this PR
|  |
|  * Some other dev commit
|  |
* This PR
| /
|/
* point at which dev was branched

@jamesbeyond
Copy link
Collaborator

All good.

The main reason for doing a reset and force push, is the main branch and dev branch are diverged , a revert commit is not helping here it will causes further diverging.

Just FYI, it doesn't matter if main and dev diverge. You would normally fix a mistake like this by git reverting it on main and then merge dev into main later, at which point your commit graph would look like this:

* Merge dev into main
|\
| \
|  * Some dev commit
|  |
|  * Some other dev commit
|  |
* Revert this PR
|  |
|  * Some other dev commit
|  |
* This PR
| /
|/
* point at which dev was branched

Hi @Timmmm,

Thanks for sharing the idea, maybe next time when diverging happens we will try this.
But in your suggested way, it will need 2 merges to get dev and main in sync.

  • merge dev to main, only bring all the commits from dev to main, at this time, main will be 2 commit ahead of dev
  • we'll need another merge bring the 2 commits form main to dev to get them in synced

But anyway, thanks for keep eye on the repo and reminding us, about the suggestions, we already put those branches with protections

@Timmmm
Copy link

Timmmm commented Jun 12, 2024

At that point you can just hard reset dev to main so they effectively share the same merge commit.

Hopefully won't be needed in future anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@allenjbaum allenjbaum allenjbaum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cmuellner @allenjbaum @Timmmm @jamesbeyond