-
Notifications
You must be signed in to change notification settings - Fork 14
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
Windows build compatibility -- Actions for Trelis 17.1 and Cubit 2021 #85
Conversation
.github/workflows/windows.yml
Outdated
shell: bash -l {0} | ||
run: | | ||
cd ${PLUGIN_ABS_PATH} | ||
git clone https://bitbucket.org/bam241/moab -b windows_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be changed after MOAB
PR gets merged.
.github/workflows/windows.yml
Outdated
run: | | ||
cd ${PLUGIN_ABS_PATH} | ||
mkdir dagmc_build dagmc_install | ||
git clone https://github.com/bam241/dagmc -b windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be changed after DAGMC
PR gets merged.
.github/workflows/windows.yml
Outdated
cp -r /d/a/Trelis-plugin/Trelis-plugin ./ | ||
cd Trelis-plugin | ||
rm -rf mcnp2cad | ||
git clone https://github.com/bam241/mcnp2cad -b windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be changed after mcnp2cad
PR gets merged.
.github/workflows/windows.yml
Outdated
-G"Visual Studio 16 2019" \ | ||
-DCubit_DIR="../Trelis/bin" \ | ||
-DCUBIT_ROOT="../Trelis/bin" \ | ||
-DUPDATE_SUBMODULES=OFF \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to be changed after mcnp2cad
PR gets merged.
5d9134a
to
85ee278
Compare
Do we use the |
Wow this will be a great addition. Super work BaM. I notice it uses the |
we never had one for windows. |
But I don't see it being used anywhere because the GH action explicitly includes all the build instructions instead |
sorry, I lost track of that file.. this is an artefact of previous work. that will need to be removed eventually... |
yes this is something we should/could consider in the future. I would like to keep this PR simple, as a first support of windows. I think we would need first to update that on the DAGMC CI.... to extend our windows capability. |
current problem seems to be on core form side... don't see any download available on their website.... maybe their server crashed.... |
I think it might be the cloud host https://downdetector.com/status/backblaze/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to choose best release actions to trigger a build
.github/workflows/windows.yml
Outdated
|
||
release: | ||
types: # This configuration does not affect the page_build event above | ||
- created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we learned last week that created
was not a good choice. It is possible to create a draft release, which then triggers this, but when you publish the release, it does not trigger again.
@shimwell , did we pick a better one? Or maybe an additional one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we preferred published
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the options are here https://docs.github.com/en/actions/reference/events-that-trigger-workflows#release
There is also a discussion here https://github.community/t/what-is-the-created-type-in-on-release/17184
@gonuke this looks good to go ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bam241 - exciting to see this getting close. Two items, only one is blocking.
.github/workflows/windows.yml
Outdated
shell: cmd | ||
run: | | ||
cd hdf | ||
msiexec.exe /i HDF5-1.8.21-win64.msi /passive /norestart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: does this mean that conda's HDF5 won't work? also the conda-forge version? I wonder if we can document this somewhere so that if/when someone tries again, they know what the problem(s) were
.github/workflows/windows.yml
Outdated
cp ../../plugin_install/bin/* ./ | ||
cp ../../plugin_install/lib/* ./ | ||
cd ${PLUGIN_ABS_PATH} | ||
tar -cvf svalinn_plugin_windows_${{ matrix.cubit }}.tar bin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we use 7z
to make a zip file instead of a tarball?
@gonuke it seems to be working, using hdf5 conda and 7x :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the ZIP format?
.github/workflows/windows.yml
Outdated
cp ../../plugin_install/bin/* ./ | ||
cp ../../plugin_install/lib/* ./ | ||
cd ${PLUGIN_ABS_PATH} | ||
7z a C:/Users/runneradmin/svalinn_plugin_windows_${{ matrix.cubit }}.7z bin\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this with a .zip
extension and ZIP format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. Thanks for figuring the rest out, @bam241 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!!! Thanks @bam241
This should not be reviewed before #84 gets merged. It builds on top of #84 and relies on some of those changes.
This also relies on multiples PR to be merged on dependencies such as
MOAB
,DAGMC
andmncp2cad
This includes:
This is NOT a standalone PR, it is depending on: