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

fix pe error by removing isgx-pe2sgx in CI #442

Merged
merged 1 commit into from
May 4, 2023
Merged

fix pe error by removing isgx-pe2sgx in CI #442

merged 1 commit into from
May 4, 2023

Conversation

Taowyoo
Copy link
Collaborator

@Taowyoo Taowyoo commented Apr 27, 2023 •
edited
Loading

Fix #433 by removing isgx-pe2sgx in CI

If it's not used, maybe we could also remove the code.

Please correct me if it's actively used somewhere

@Taowyoo Taowyoo marked this pull request as draft April 27, 2023 22:07
@Taowyoo Taowyoo requested a review from jethrogb April 27, 2023 22:08
@Taowyoo Taowyoo marked this pull request as ready for review April 27, 2023 22:10
@Taowyoo Taowyoo requested review from Pagten and arai-fortanix April 27, 2023 22:10
@Taowyoo Taowyoo changed the title fix pe error by removing isgx-pe2sgx bin fix pe error by removing isgx-pe2sgx in CI Apr 27, 2023
@arai-fortanix
Copy link
Contributor

I think we probably still want cargo test --verbose --locked -p sgxs-tools, but we can get rid of the --features pe2sgxs --bin isgx-pe2sgx part of that command.

@jethrogb
Copy link
Member

That test is already covered by this line

@jethrogb
Copy link
Member

Let's move the code to a “deprecated” directory or something like that and add a README to make clear that this isn't tested.

@Taowyoo
Copy link
Collaborator Author

Taowyoo commented Apr 28, 2023 via email •
edited
Loading

@Taowyoo Taowyoo force-pushed the yx/fix-pe-compile-error branch from 2ee5c37 to bd29e72 Compare April 28, 2023 16:23
@arai-fortanix
Copy link
Contributor

I think Jethro also wanted the isgx-pe2sgx.rs file moved to a deprecated directory, in addition to the README update, so it's more obvious that it's no longer being maintained.

@Taowyoo
Copy link
Collaborator Author

Taowyoo commented May 4, 2023 •
edited
Loading

  • move isgx-pe2sgx.rs to deprecated folder
  • remove sgxs-tools 's feature pe2sgxs
  • update sgxs-tools 's README

Since it seems isgx-pe2sgx is not used externally, I assume I do not need to bump minor version

arai-fortanix
arai-fortanix approved these changes May 4, 2023
View reviewed changes
Copy link
Contributor

@arai-fortanix arai-fortanix left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Taowyoo
remove pe2sgxs
533914b 
- remove CI for pe2sgxs
- update `sgxs-tools` 's README
- move `isgx-pe2sgx.rs` to deprecated folder
- remove `sgxs-tools` 's feature `pe2sgxs` and related dependencies
@Taowyoo Taowyoo force-pushed the yx/fix-pe-compile-error branch from d975291 to 533914b Compare May 4, 2023 20:29
@Taowyoo
Copy link
Collaborator Author

Taowyoo commented May 4, 2023 •
edited
Loading

squashed all commits into one

@Taowyoo
Copy link
Collaborator Author

Taowyoo commented May 4, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented May 4, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 11e11c0 into master May 4, 2023
Taowyoo added a commit that referenced this pull request May 30, 2023
@bors @Taowyoo
Merge #442
c78510d 
442: fix pe error by removing `isgx-pe2sgx` in CI r=Taowyoo a=Taowyoo

Fix #433 by removing `isgx-pe2sgx` in CI

If it's not used, maybe we could also remove the code.

Please correct me if it's actively used somewhere

Co-authored-by: Yuxiang Cao <yuxiang.cao@fortanix.com>
@Taowyoo Taowyoo deleted the yx/fix-pe-compile-error branch September 7, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arai-fortanix arai-fortanix arai-fortanix approved these changes

@raoulstrackx raoulstrackx Awaiting requested review from raoulstrackx

@jethrogb jethrogb Awaiting requested review from jethrogb

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

Successfully merging this pull request may close these issues.

sgxs-tools failed to compile when using feature pe2sgxs on nightly
3 participants
@Taowyoo @arai-fortanix @jethrogb